-
Notifications
You must be signed in to change notification settings - Fork 42
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
Make HttpURLConnection connection inactivity timeout configurable and add test for harvester code #569
Make HttpURLConnection connection inactivity timeout configurable and add test for harvester code #569
Conversation
* <p>Default value: "10000" | ||
*/ | ||
public static void setConnectionInactivityTimeoutMs(long timeoutMs) { | ||
HttpUrlInstrumentationConfig.connectionInactivityTimeoutMs = timeoutMs; |
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.
should we check if the value here is at least positive and non zero?
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.
It does make sense for it to be positive for production workflows. I can throw an IllegalArgumentException if it's negative or zero. Does that make sense?
We will need another test method then, similar to this where we allow for negative values:
public static void setConnectionInactivityTimeoutMsForTesting(long timeoutMs) {
HttpUrlInstrumentationConfig.connectionInactivityTimeoutMs = timeoutMs;
}
If the customer wants to test the harvester with this they can as well use it, so no issue in exposing this to the customer?
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.
Conventionally, zero has been used for a long time to mean "no timeout, wait forever". Not sure how much we need or care about that here. Just throwing an exception for negative seems reasonable to me.
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 an exception for negative values! Also added a test only function to allow negative values for test, so wait times are not needed in test.
Added some good documentation to prevent misuse. We can also add @VisibleForTesting annotation on the test function. Right now held off on that as I think we're not using the annotation beyond documentation purposes and we need to introduce additional dependency in this module for just one function. Let me know if it's better to have that annotation instead OR a 1ms wait time in the test functionality instead of this additional function.
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.
Just a small nit, but looks good to me otherwise.
Co-authored-by: jason plumb <[email protected]>
…ethod to allow negative value.
Made connection inactivity timeout ms configurable and added a test case for harvester code.
Next items for future PRs: