HADOOP-18463. Add an integration test process data asynchronously during vectored read. #4921
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
…ing vectored read. part of HADOOP-18103.
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Sep 22, 2022
| } | ||
| }); | ||
| } | ||
| if (!countDown.await(100, TimeUnit.SECONDS)) { |
There was a problem hiding this comment.
timeout should be a static constant and more visible
| }); | ||
| } | ||
| if (!countDown.await(100, TimeUnit.SECONDS)) { | ||
| throw new AssertionError("Error while processing vectored io results"); |
| try { | ||
| readBufferValidateDataAndReturnToPool(pool, res, countDown); | ||
| } catch (Exception e) { | ||
| LOG.error("Error while process result for {} ", res, e); |
There was a problem hiding this comment.
should be saved to a field/variable, with junit thread rethrowing if the value is non null
| } | ||
| } finally { | ||
| pool.release(); | ||
| HadoopExecutors.shutdown(dataProcessor, LOG, 100, TimeUnit.SECONDS); |
There was a problem hiding this comment.
use same constant as proposed for L100
| throw new AssertionError("Error while processing vectored io results"); | ||
| } | ||
| } finally { | ||
| pool.release(); |
There was a problem hiding this comment.
how about adding an assert on L408 that the pool has its buffers returned?
There was a problem hiding this comment.
Actually I shouldn't be calling pool.release() here as it is a shared pool by all the tests. Initially I started with a new pool for this specific test then moved to the common one.
| CountDownLatch countDownLatch) | ||
| throws IOException, TimeoutException { | ||
| CompletableFuture<ByteBuffer> data = res.getData(); | ||
| ByteBuffer buffer = FutureIO.awaitFuture(data, |
There was a problem hiding this comment.
I think we all -you, me, everyone else- needs to spend some time working with CompletableFuture and chaining them.
In this code
data.thenAccept(buffer -> {
// all the validation
});
and await() for that.
It's a mess because java's checked exceptions cripple their lambda-expression methods when IO operations are invoked. But if we trying to live in their world at least we will get more insight into how we could actually improve our own code to work better there. Though it may of course be too late by now.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
approved these changes
Sep 27, 2022
| "vecRead", buffer, res.getLength(), DATASET); | ||
| // return buffer to the pool once read. | ||
| pool.putBuffer(buffer); | ||
| }), VECTORED_READ_OPERATION_TEST_TIMEOUT_SECONDS, TimeUnit.SECONDS); |
There was a problem hiding this comment.
nit: put the timeout + unit on a new line
| } | ||
| } finally { | ||
| pool.release(); | ||
| HadoopExecutors.shutdown(dataProcessor, LOG, 100, TimeUnit.SECONDS); |
There was a problem hiding this comment.
use VECTORED_READ_OPERATION_TEST_TIMEOUT_SECONDS,
|
🎊 +1 overall
This message was automatically generated. |
mukund-thakur
added a commit
that referenced
this pull request
Sep 28, 2022
…during vectored read. (#4921) part of HADOOP-18103. Contributed by: Mukund Thakur
mukund-thakur
added a commit
that referenced
this pull request
Sep 28, 2022
…during vectored read. (#4921) part of HADOOP-18103. Contributed by: Mukund Thakur
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…during vectored read. (apache#4921) part of HADOOP-18103. Contributed by: Mukund Thakur
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
part of HADOOP-18103.
Description of PR
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?