-
Notifications
You must be signed in to change notification settings - Fork 9k
HADOOP-19575. ABFS: [FNSOverBlob] Add Distinct String In User Agent to Get Telemetry for FNS-Blob #7713
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
base: trunk
Are you sure you want to change the base?
Conversation
@@ -355,6 +356,25 @@ public void verifyUserAgentClusterType() throws Exception { | |||
.contains(DEFAULT_VALUE_UNKNOWN); | |||
} | |||
|
|||
@Test | |||
// Test to verify that the user agent string for FNS-Blob accounts | |||
public void verifyUserAgentForFNSBlob() throws Exception { |
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 similar test for dfs and validate that no extra string is added.
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.
Added
🎊 +1 overall
This message was automatically generated. |
Test Results============================================================
|
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.
+1
Thanks for the patch. LGTM
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@Test | ||
// Test to verify that the user agent string for non-FNS-Blob accounts | ||
// does not contain the FNS identifier. | ||
public void verifyUserAgentForDFS() throws Exception { |
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.
assumeHnsDisabled() as well
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.
For both HNS-DFS and FNS-DFS we would want to check that the string isnt added right
@@ -152,6 +153,7 @@ public abstract class AbfsClient implements Closeable { | |||
public static final Logger LOG = LoggerFactory.getLogger(AbfsClient.class); | |||
public static final String HUNDRED_CONTINUE_USER_AGENT = SINGLE_WHITE_SPACE + HUNDRED_CONTINUE + SEMICOLON; | |||
public static final String ABFS_CLIENT_TIMER_THREAD_NAME = "abfs-timer-client"; | |||
public static final String FNS_BLOB_USER_AGENT_IDENTIFIER = "FNS"; |
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.
Any reason for just facing FNS as value for FNS Blob user agent? FNS user agent can be there for FNS DFS as well, right?
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.
We are adding the string only for FNS-Blob cases (since we wanted to check their usage only)
Initially we added "FNS-Blob" as string but then decided to shorten it to "FNS" since the endpoint we can check from the URL itself
@Test | ||
// Test to verify the unique identifier in user agent string for FNS-Blob accounts | ||
public void verifyUserAgentForFNSBlob() throws Exception { | ||
Assume.assumeTrue(JDK_HTTP_URL_CONNECTION == httpOperationType); |
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.
Why this assume for connection type?
Description of PR
JIRA: https://issues.apache.org/jira/browse/HADOOP-19575
Adding a unique identifier in the user agent for FNS-Blob case for telemetry purposes.
How was this patch tested?
Test suite was run- the results of which are added in the comments below