-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDFS-15168: ABFS enhancement to translate AAD Object to Linux idenities #1858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@virajith, @ThomasMarquardt, @bilaharith - Please review. |
|
Integration Test results (after the javadoc and style fixes) - East US2: |
steveloughran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
design looks good, as do javadocs on text format. Some tuning needed before it's ready to go in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a "." to the end of each sentence, here and below, to keep javadoc happy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a space between the java. and the org.apache block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add trailing "."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add trailing "."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the other comments about import ordering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this.
2. put the ? on a new line; the : should also go there; the ?: sequence is significant enough to highligh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be very noisy if it happens a lot. Is that what you want?
also: it doesn't provide the information you'd need to debug the failure from the logs
Preferred
- Error message includes string being looked up and resolved line.
- full stack at DEBUG only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit "."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see all comments in previous block and apply here as appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- add use of "#" as comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveloughran, clarification - you would want # with <pre> block or just #?
* Group identity file should be delimited by colon in below format.
* <pre>
* # OBJ_ID:GROUP_NAME:GROUP_ID:SGP_NAME
* </pre>
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ABFS driver enhancement - Allow customizable translation from AAD SPNs and security groups to Linux user and group
Integration Test results - East US2:
Tests: 1255 Errors: 0 Failures: 0 Skipped: 390`