-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-18957. Use StandardCharsets.UTF_8 #6231
Conversation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
more more Update NativeAzureFileSystem.java more more more more
7524b88
to
3493efc
Compare
💔 -1 overall
This message was automatically generated. |
@steveloughran @ayushtkn @slfan1989 Would any of you have time to look at this PR? |
I will be back by end of next week, if nobody volunteers by then, I can take of this :-) |
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.
Quick Pass & mostly looks good, can we have some restrictions to prevent future usage of these?
Something like this or better?
https://github.com/apache/hadoop/blob/trunk/pom.xml#L194
Good idea. I've committed a banned import for the Guava Charsets class. |
🎊 +1 overall
This message was automatically generated. |
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.
Looks good almost, minor question,
anybody else plans to review this? let me know if anyone wants me to hold merging this or needs time
...adoop-common/src/main/java/org/apache/hadoop/security/token/delegation/web/ServletUtils.java
Outdated
Show resolved
Hide resolved
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.
LGTM
💔 -1 overall
This message was automatically generated. |
yetus reports some checksyle warnings |
@ayushtkn do you have tips and tricks for how to find checkstyle issues caused by a PR? This PR is pretty big touching 100s of files and there are 1000s of pre-existing checkstyle issues. I tried concentrating on going through LineLength and UnusedImports issues but there are 100s of them and after spending a while on them I hadn't yet reached one that was caused by me. To be honest, I'm not really in a good position time wise to read through such a large checkstyle output. I code mainly in Scala and tools like scalafmt will not only spot checkstyle issues, it will fix them too. |
There is a link with the checkstyle warnings, just click on it Some 11 lines
|
The list of 11 checkstyle issues are largely pre-existing issues With the indentation issues, the highlighted lines match the surrounding lines in terms of style. With the In 218321b, I have tried to fix one line length issue and tried to fix a pre-existing indentation issue in a test. |
💔 -1 overall
This message was automatically generated. |
Failed test passes locally
|
…y PJ Fanning. Signed-off-by: Ayush Saxena <[email protected]>
Description of PR
HADOOP-18957
also tidies up some cases like
new String("xyz")
and just uses"xyz"
.How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?