-
Notifications
You must be signed in to change notification settings - Fork 425
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
measured input stream #262
base: master
Are you sure you want to change the base?
Conversation
💔 -1 overall
This message was automatically generated. |
@@ -563,7 +564,7 @@ private HostFetchResult setupConnection(Collection<InputAttemptIdentifier> attem | |||
|
|||
protected void setupConnectionInternal(String host, Collection<InputAttemptIdentifier> attempts) | |||
throws IOException, InterruptedException { | |||
input = httpConnection.getInputStream(); | |||
input = new MeasuredDataInputStream(httpConnection.getInputStream()); |
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 for making it the default. Note that System.currentTimeMillis may turn out to be expensive when number of calls are higher. Especially in cloud nodes where hypervisors are involved.
Would suggest to make it optional.
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 for the input @rbalamohan, yes, absolutely, this is a work-in-progress patch, I was thinking about making it configurable too...working on that
I hope the basic idea makes sense to you: counting the time spent in InputStream.read to reflect the actual time spent with network traffic
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 didn't realize that it was WIP patch. sure, +1 on the idea.
No description provided.