-
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-11492. Improve createJerseyClient#setConnectTimeout Code. #5636
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
|
||
long connectTimeOut = conf.getTimeDuration(YarnConfiguration.ROUTER_WEBAPP_CONNECT_TIMEOUT, | ||
YarnConfiguration.DEFAULT_ROUTER_WEBAPP_CONNECT_TIMEOUT, TimeUnit.MILLISECONDS); | ||
long readTimeout = conf.getTimeDuration(YarnConfiguration.ROUTER_WEBAPP_READ_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.
Let's cast it to int right here?
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.
Thank you for your help to review the code! I will modify the code.
connectTimeOut = TimeUnit.MILLISECONDS.convert( | ||
YarnConfiguration.DEFAULT_ROUTER_WEBAPP_CONNECT_TIMEOUT, TimeUnit.MILLISECONDS); | ||
} | ||
client.setConnectTimeout((int) connectTimeOut); |
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.
Let's add units to these vars.
YarnConfiguration configuration = new YarnConfiguration(); | ||
Client client01 = RouterWebServiceUtil.createJerseyClient(configuration); | ||
Map<String, Object> properties = client01.getProperties(); | ||
Object readTimeOut = properties.get(ClientConfig.PROPERTY_READ_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.
Let's cast.
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.
I will modify the code.
🎊 +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. |
// Case3, Set the maximum value that exceeds the integer | ||
// We'll get the default timeout(30s) | ||
YarnConfiguration configuration3 = new YarnConfiguration(); | ||
long connectTimeOutLong = (long) Integer.MAX_VALUE + 1; | ||
long readTimeOutLong = (long) Integer.MAX_VALUE + 1; |
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 it is
long connectTimeOutLong = (long) Integer.MAX_VALUE * 3L;
long readTimeOutLong = (long) Integer.MAX_VALUE * 3L;
you won't get the default value I think, because you are relying on the value to go negative when you cast, but post cast it can go +ve 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.
Thank you for helping to review the code! I agree with your point that if users set a very large value for the timeout, it could indeed lead to unexpected timeouts. Generally, this timeout value should not exceed 1 minute. If the user sets a value much larger than this, I will print a warning log to alert them.
Could this approach work?
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.
I think the correct approach would be first get the long, figure out whether it is -ve or bigger than Integer.MAX and if so put a log and use the default.
If it is within limit, then may be we can do a cast
🎊 +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
Description of PR
JIRA: YARN-11492. Improve createJerseyClient#setConnectTimeout Code.
createJerseyClient#setConnectTimeout There is a type conversion code that converts long to int, we should be careful with this part of code, I added some verification rules.
How was this patch tested?
Add unit tests.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?