HADOOP-18320. Fixes typos in Delegation Tokens documentation. #4499
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
dannycjones
suggested changes
Jun 29, 2022
There was a problem hiding this comment.
Thanks @ahmarsuhail, looking good. I have a few some comments where we can improve further.
Can you create a JIRA for this work and update the title to include the ID and be specific to S3A?
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/delegation_tokens.md
Outdated
Show resolved
Hide resolved
Comment on lines
+63
to
+64
| Tokens are opaque to clients. Clients simply get a byte array | ||
| of data which they must provide to a service when required. |
There was a problem hiding this comment.
Since we are touching this line, add a line break before "Clients simply" to keep sentence diffs independent in future.
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/delegation_tokens.md
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/delegation_tokens.md
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/delegation_tokens.md
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/delegation_tokens.md
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/delegation_tokens.md
Show resolved
Hide resolved
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/delegation_tokens.md
Outdated
Show resolved
Hide resolved
Comment on lines
+111
to
113
| Issued tokens may be included in job submissions, passed to running applications, | ||
| etc. This token is specific to an individual bucket; all buckets which a client | ||
| wishes to work with must have a separate delegation token issued. |
There was a problem hiding this comment.
I find this confusing in the context of delegated role tokens, where the token is literally specific to an individual bucket.
Can we make this clearer while we are editing this paragraph?
Suggested change
| Issued tokens may be included in job submissions, passed to running applications, | |
| etc. This token is specific to an individual bucket; all buckets which a client | |
| wishes to work with must have a separate delegation token issued. | |
| Issued tokens may be included in job submissions, passed to running applications, | |
| etc. | |
| Delegation tokens are specific to an S3A filesystem. | |
| Each filesystem a client wishes to work with must have a separate delegation token issued, | |
| regardless of the delegation token variant. |
Is it correct to describe them specific to a filesystem rather than the bucket?
There was a problem hiding this comment.
Discussed offline - will leave it as is for now as the suggestion wouldn't be making it clearer.
ahmarsuhail
changed the title
Co-authored-by: Daniel Carl Jones <danny@danielcarl.info>
dannycjones
approved these changes
Jun 30, 2022
|
🎊 +1 overall
This message was automatically generated. |
|
@mehakmeet / @mukund-thakur I'd made some minor updates to the delegation tokens documentation a while ago, would you be able to take a look? |
| @@ -108,7 +108,7 @@ password-protected data opaque to clients; they contain the secrets needed | |||
| to access the relevant S3 buckets and associated services. | |||
|
|
|||
| They are obtained by requesting a delegation token from the S3A filesystem client. | |||
There was a problem hiding this comment.
Not sure why github not allowing me to comment at L82.
Delegation "Token" support is
There was a problem hiding this comment.
L106 -> These S3A Delegation Tokens are special in "a way" that
| @@ -353,10 +352,10 @@ it is authenticated with; the role token binding will fail. | |||
|
|
|||
| When the AWS credentials supplied to the Session Delegation Token binding | |||
| through `fs.s3a.aws.credentials.provider` are themselves a set of | |||
There was a problem hiding this comment.
Something is off here, maybe "they themselves are part of session credentials."
There was a problem hiding this comment.
this seems fine to me as is. Maybe removing the themselves entirely would help, so it becomes "through fs.s3a.aws.credentials.provider are a set of", but personally feel it's clearer as is.
|
thanks for taking a look @mukund-thakur, have made the suggested changes. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Noticed some typos while reading through the documentation, this PR fixes those.
How was this patch tested?
Not required