-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HADOOP-18186. s3a prefetching to use SemaphoredDelegatingExecutor for submitting work #4796
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
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
Endpoint |
|
With When ran individually, |
|
@steveloughran i also tested the entire test suite by enabling prefatch. Tests in However, the failures don't seem relevant to the change on this PR. As per the test logic, it seems |
| unboundedThreadPool.allowCoreThreadTimeOut(true); | ||
| executorCapacity = intOption(conf, | ||
| EXECUTOR_CAPACITY, DEFAULT_EXECUTOR_CAPACITY, 1); | ||
| if (this.prefetchEnabled) { |
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.
can you cut the this. from here and the line below. not something we do in the hadoop code except in constructors and setters
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.
sounds good, done
|
💔 -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.
+1 looks good, Only suggestion is maybe a test in prefetch IT to verify the duration for executor acquisition we do in SemaphoredDelegatingExecutor for input stream IOStats.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
With |
|
Addressed review comments |
|
@steveloughran @mehakmeet could you please take another look? |
|
i'm letting mehakmeet review this |
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, just a small doubt on the assertion.
| } | ||
| // Verify that once stream is closed, all memory is freed | ||
| verifyStatisticGaugeValue(ioStats, StreamStatisticNames.STREAM_READ_ACTIVE_MEMORY_IN_USE, 0); | ||
| assertDurationRange(ioStats, StoreStatisticNames.ACTION_EXECUTOR_ACQUIRED, 0, 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.
This would pass for the duration of acquisition executor=0 as well. Think having an assertThat.isGreaterThan(0) should be fine 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.
Done
|
🎊 +1 overall
This message was automatically generated. |
|
Tested against us-west-2 endpoint. |
|
@mehakmeet the PR is updated based on review comments |
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
… submitting work (apache#4796) Contributed by Viraj Jasani
… submitting work (#4796) Contributed by Viraj Jasani
… submitting work (#4796) Contributed by Viraj Jasani
… submitting work (#4796) Contributed by Viraj Jasani
… submitting work (#4796) Contributed by Viraj Jasani
No description provided.