-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
YARN-11634. [Addendum] Speed-up TestTimelineClient. #6419
Conversation
@brumi1024 @K0K0V0K In #6371, we introduced a sputbug. I tried to modify the code, can you help review this PR? Thank you very much!
|
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -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.
Thanks @slfan1989 for the patch. I have one small comment.
@@ -145,14 +145,14 @@ protected void serviceInit(Configuration conf) throws Exception { | |||
@Override | |||
public HttpURLConnection configure(HttpURLConnection conn) | |||
throws IOException { | |||
setTimeouts(conn, DEFAULT_SOCKET_TIMEOUT); | |||
setTimeouts(conn, 60_000); |
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: the behaviour changed a bet, if someone simply edits the socketTimeout variable, this won't be changed with it. Shouldn't the socketTimeout be used here 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.
@brumi1024 Thanks for reviewing the code, I will improve it.
@@ -89,7 +89,7 @@ public void tearDown() throws Exception { | |||
if (isSSLConfigured()) { | |||
KeyStoreTestUtil.cleanupSSLConfig(keystoresDir, sslConfDir); | |||
} | |||
TimelineConnector.DEFAULT_SOCKET_TIMEOUT = 60_000; | |||
client.getConnector().setSocketTimeOut(60_1000); |
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.
Hi @slfan1989 !
Thanks for the patch!
If i see well we have a typo here, 60_1000 seems too much.
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.
@K0K0V0K Thanks for the review! I will improve it.
💔 -1 overall
This message was automatically generated. |
@brumi1024 @K0K0V0K Can you help review the code again? Thank you very much! |
💔 -1 overall
This message was automatically generated. |
Thanks @slfan1989 for this fix. LGTM ! |
💔 -1 overall
This message was automatically generated. |
@ayushtkn @Hexiaoqiao @brumi1024 This PR should also need to be merged before Can you help me review this PR? Thank you very much!
|
@brumi1024 Can you help review this PR again? Thank you very much! |
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.
Thanks @slfan1989, the latest state LGTM. Merging to trunk.
@brumi1024 Thank you for reviewing the code! |
Co-authored-by: slfan1989 <slfan1989@apache.org>
Co-authored-by: slfan1989 <slfan1989@apache.org>
Co-authored-by: slfan1989 <slfan1989@apache.org>
Description of PR
JIRA: YARN-11634. [Addendum] Speed-up TestTimelineClient.
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?