-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-16922. ABFS: Change User-Agent header #1938
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
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Driver test results using accounts in Central India Account with HNS Support Account without HNS support |
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.
overall it looks good to me, but plz fix the checkstyle issues reported in the Yetus.
Could you also provide some background why this change is needed and will it affect the existing customers?
...-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/services/TestAbfsClient.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +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.
- This could be pulled out of AbfsClient or made package-protected static and so be easier to test
- Is there a regexp somewhere of a well-formed HTTP UA header must look like? If so, test should verify it.
@@ -679,32 +679,60 @@ public AuthType getAuthType() { | |||
|
|||
@VisibleForTesting | |||
String initializeUserAgent(final AbfsConfiguration abfsConfiguration, | |||
final String sslProviderName) { | |||
final String sslProviderName) { |
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.
if you pulled this out somewhere or made static you could have a unit test which would validate some 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.
There are test cases written to validate this as this method is package protected and with the @VisibleForTesting.
StringBuilder sb = new StringBuilder(); | ||
sb.append("(JavaJRE "); | ||
|
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.
can we have org.apache.hadoop.util.VersionInfo,getVersion() in there too? Lets people pin the blame on the logs
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.
CLIENT_VERSION includes org.apache.hadoop.util.VersionInfo,getVersion()
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.
Review comments addressed
Driver test results using accounts in Central India Account with HNS Support Account without HNS support |
🎊 +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.
LGTM, +1
@steveloughran do you have pending request for changes?
@DadanielZ I'm happy if you are. |
At present the User-Agent header would look like: With this PR it would be: |
Contributed by Bilahari T H. (cherry picked from commit 264e49c)
Contributed by Bilahari T H.
Contributed by Bilahari T H. Conflicts: hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/constants/FileSystemConfigurations.java Change-Id: Icc968be7234dcf93f352280c056d645a9439c459
HADOOP-16922. Changing User-Agent header.
At present the User-Agent header would look like:
Azure Blob FS/3.4.0-SNAPSHOT (JavaJRE 1.8.0_241; Linux 5.3.0-46-generic; SunJSSE-1.8) MSFT
With this PR it would be:
APN/1.0 Azure Blob FS/3.4.0-SNAPSHOT (OracleCorporation JavaJRE 1.8.0_241; Linux 5.3.0-46-generic/amd64; SunJSSE-1.8; UNKNOWN/UNKNOWN) MSFT