-
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. Speed-up TestTimelineClient #6371
Conversation
The TimelineConnector.class has a hardcoded 1 minute connection time out, what makes the TestTimelineClient to a long running test (~15:30 min). Decrease the timeout to 10ms will speed up the test run (~56 sec).
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 @K0K0V0K for the patch, in general it looks good to me, I just had one small question.
@@ -78,6 +78,7 @@ public void setup() { | |||
conf.setBoolean(YarnConfiguration.TIMELINE_SERVICE_ENABLED, true); | |||
conf.setFloat(YarnConfiguration.TIMELINE_SERVICE_VERSION, 1.0f); | |||
client = createTimelineClient(conf); | |||
TimelineConnector.DEFAULT_SOCKET_TIMEOUT = 10; |
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.
Do you see a chance that test could fail due to a quite low socket timeout?
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.
well... it is a good question that made me wonder in case of a successful connection how long the KerberosAuthenticator#190 connect is running, and the plot-twist i just realized all of the tests expect timeout here 😅
If you add a some debug point before and after this line we can observe the debug point after the connect will never be triggered, so technically we can also write 1ms here, the tests can not be broken
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, in that case LGTM.
💔 -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 @K0K0V0K for the update, LGTM.
@@ -78,6 +78,7 @@ public void setup() { | |||
conf.setBoolean(YarnConfiguration.TIMELINE_SERVICE_ENABLED, true); | |||
conf.setFloat(YarnConfiguration.TIMELINE_SERVICE_VERSION, 1.0f); | |||
client = createTimelineClient(conf); | |||
TimelineConnector.DEFAULT_SOCKET_TIMEOUT = 10; |
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, in that case LGTM.
Merging to trunk. |
The TimelineConnector.class has a hardcoded 1 minute connection time out, what makes the TestTimelineClient to a long running test (~15:30 min). Decrease the timeout to 10ms will speed up the test run (~56 sec).
Description of PR
The TimelineConnector.class has a hardcoded 1 minute connection time out, what makes the TestTimelineClient to a long running test (~15:30 min). Decrease the timeout to 10ms will speed up the test run (~56 sec).
How was this patch tested?