HADOOP-18546. Followup: ITestReadBufferManager fix #5198
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a followup to the original HADOOP-18546 patch; cherry-picks of that should include this or follow up with it. Removes risk of race conditions in assertions of ITestReadBufferManager on the state of the in-progress and completed queues by removing the assertions. * Assertions that there are no in progress reads MUST be cut as there may be some and they won't be cancelled. * Assertions that the completed list is without buffers of a closed stream are brittle because if there was an in progress stream which completed after stream.close() then it will end up in the list. Change-Id: Ide2570db36b0f6c23a6569d90446de28cebfc2ac
|
azure endpoint is cardiff; parallel -Dscale run in progress. already verified the failing test seems happy now. (given i've deleted assertions, it'd be hard not to |
|
💔 -1 overall
This message was automatically generated. |
saxenapranav
reviewed
Dec 9, 2022
| } | ||
| } finally { | ||
| executorService.shutdown(); | ||
| } |
There was a problem hiding this comment.
executorService.shutdown does an orderly shutdown of the task. It does not wait for the tasks to be completed. So, the main thread after executing line 76 will go to line 79, and assertions will happen where the execution of tasks may or may not have got completed.
Requesting you to kindly change to https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html#awaitTermination method which will wait for the tasks to be completed. Thanks.
Ref: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html#shutdown()
There was a problem hiding this comment.
aah. yes, will do that. we still need to cut the assertions about completed list and buffers returned, as any in-progress read will finish and add the buffer which will then stay until the threshold timeout
Change-Id: Icc9940851789d0796bc4ef8467093aa3f597ee4e
|
ok, updated this one, which i'm using in my internal cherrypick which I need for e2e testing though spark. will review your #5199 change which does more than just comment out the failing assertions. my patch tested against azure cardiff |
|
💔 -1 overall
This message was automatically generated. |
|
not happy about javadoc failures, but looks unrelated (annotation issues? javadoc changes?). going to approve this myself because it's only a test change, and the change is just removing some flaky asserts plus the fix from @pranavsaxena-microsoft . If I'd checked out the PR and tested locally before the merge i would have just included them in the original commit. +1 |
asfgit
pushed a commit
that referenced
this pull request
Dec 9, 2022
… stream close() (#5176) This addresses HADOOP-18521, "ABFS ReadBufferManager buffer sharing across concurrent HTTP requests" by not trying to cancel in progress reads. It supercedes HADOOP-18528, which disables the prefetching. If that patch is applied *after* this one, prefetching will be disabled. As well as changing the default value in the code, core-default.xml is updated to set fs.azure.enable.readahead = true As a result, if Configuration.get("fs.azure.enable.readahead") returns a non-null value, then it can be inferred that it was set in or core-default.xml (the fix is present) or in core-site.xml (someone asked for it). Note: this commit contains the followup commit: HADOOP-18546. Followup: ITestReadBufferManager fix (#5198) That is needed to avoid race conditions in the test. Contributed by Pranav Saxena.
Hi @steveloughran , Thanks a lot for the PR. Related to javadocs failure, it seems that it is bit inconsistent. Sometimes it fails and then back-merging the trunk, the test passes. |
|
yeah, nothing related to this pr. been mentioned elsewhere and i think it's java 11 related. moving to a static import of the specific values will apparently fix, but this should be a standalone "fix javadocs" pr for each module affected |
slfan1989
pushed a commit
to slfan1989/hadoop
that referenced
this pull request
Dec 20, 2022
This is a followup to the original HADOOP-18546 patch; cherry-picks of that should include this or follow up with it. Removes risk of race conditions in assertions of ITestReadBufferManager on the state of the in-progress and completed queues by removing assertions brittle to race conditions in scheduling/network IO * Waits for all the executor pool shutdown to complete before making any assertions * Assertions that there are no in progress reads MUST be cut as there may be some and they won't be cancelled. * Assertions that the completed list is without buffers of a closed stream are brittle because if there was an in progress stream which completed after stream.close() then it will end up in the list. Contributed by Steve Loughran
anmolanmol1234
pushed a commit
to ABFSDriver/AbfsHadoop
that referenced
this pull request
Mar 6, 2023
This is a followup to the original HADOOP-18546 patch; cherry-picks of that should include this or follow up with it. Removes risk of race conditions in assertions of ITestReadBufferManager on the state of the in-progress and completed queues by removing assertions brittle to race conditions in scheduling/network IO * Waits for all the executor pool shutdown to complete before making any assertions * Assertions that there are no in progress reads MUST be cut as there may be some and they won't be cancelled. * Assertions that the completed list is without buffers of a closed stream are brittle because if there was an in progress stream which completed after stream.close() then it will end up in the list. Contributed by Steve Loughran
anmolanmol1234
pushed a commit
to ABFSDriver/AbfsHadoop
that referenced
this pull request
Apr 3, 2023
This is a followup to the original HADOOP-18546 patch; cherry-picks of that should include this or follow up with it. Removes risk of race conditions in assertions of ITestReadBufferManager on the state of the in-progress and completed queues by removing assertions brittle to race conditions in scheduling/network IO * Waits for all the executor pool shutdown to complete before making any assertions * Assertions that there are no in progress reads MUST be cut as there may be some and they won't be cancelled. * Assertions that the completed list is without buffers of a closed stream are brittle because if there was an in progress stream which completed after stream.close() then it will end up in the list. Contributed by Steve Loughran
anujmodi2021
pushed a commit
to anujmodi2021/hadoop
that referenced
this pull request
Apr 3, 2023
This is a followup to the original HADOOP-18546 patch; cherry-picks of that should include this or follow up with it. Removes risk of race conditions in assertions of ITestReadBufferManager on the state of the in-progress and completed queues by removing assertions brittle to race conditions in scheduling/network IO * Waits for all the executor pool shutdown to complete before making any assertions * Assertions that there are no in progress reads MUST be cut as there may be some and they won't be cancelled. * Assertions that the completed list is without buffers of a closed stream are brittle because if there was an in progress stream which completed after stream.close() then it will end up in the list. Contributed by Steve Loughran
anujmodi2021
pushed a commit
to anujmodi2021/hadoop
that referenced
this pull request
Apr 3, 2023
This is a followup to the original HADOOP-18546 patch; cherry-picks of that should include this or follow up with it. Removes risk of race conditions in assertions of ITestReadBufferManager on the state of the in-progress and completed queues by removing assertions brittle to race conditions in scheduling/network IO * Waits for all the executor pool shutdown to complete before making any assertions * Assertions that there are no in progress reads MUST be cut as there may be some and they won't be cancelled. * Assertions that the completed list is without buffers of a closed stream are brittle because if there was an in progress stream which completed after stream.close() then it will end up in the list. Contributed by Steve Loughran
anujmodi2021
pushed a commit
to anujmodi2021/hadoop
that referenced
this pull request
Apr 4, 2023
This is a followup to the original HADOOP-18546 patch; cherry-picks of that should include this or follow up with it. Removes risk of race conditions in assertions of ITestReadBufferManager on the state of the in-progress and completed queues by removing assertions brittle to race conditions in scheduling/network IO * Waits for all the executor pool shutdown to complete before making any assertions * Assertions that there are no in progress reads MUST be cut as there may be some and they won't be cancelled. * Assertions that the completed list is without buffers of a closed stream are brittle because if there was an in progress stream which completed after stream.close() then it will end up in the list. Contributed by Steve Loughran
anujmodi2021
pushed a commit
to anujmodi2021/hadoop
that referenced
this pull request
Apr 4, 2023
This is a followup to the original HADOOP-18546 patch; cherry-picks of that should include this or follow up with it. Removes risk of race conditions in assertions of ITestReadBufferManager on the state of the in-progress and completed queues by removing assertions brittle to race conditions in scheduling/network IO * Waits for all the executor pool shutdown to complete before making any assertions * Assertions that there are no in progress reads MUST be cut as there may be some and they won't be cancelled. * Assertions that the completed list is without buffers of a closed stream are brittle because if there was an in progress stream which completed after stream.close() then it will end up in the list. Contributed by Steve Loughran
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a followup to the original HADOOP-18546
patch (#5176); cherry-picks of that should include this
or follow up with it.
Removes risk of race conditions in assertions
of ITestReadBufferManager on the state of the in-progress and completed queues by removing the assertions.
How was this patch tested?
modified test run standalone; doing full test suite as well for due diligence
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?