HADOOP-18959 S3A : Use builder for prefetch CachingBlockManager #6240
Conversation
This file contains 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
|
Tested against
|
|
💔 -1 overall
This message was automatically generated. |
|
@ahmarsuhail could you please review this PR? |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ahmarsuhail
requested changes
Nov 13, 2023
| * This class is used to provide params to {@link BlockManager}. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public final class BlockManagerParams { |
There was a problem hiding this comment.
nit: could you please rename to BlockManagerParameters ?
| /** | ||
| * Asynchronous tasks are performed in this pool. | ||
| */ | ||
| private final ExecutorServiceFuturePool futurePool; |
There was a problem hiding this comment.
nit: could you add a new line after each of these, makes for slightly easier reading.
| private final DurationTrackerFactory trackerFactory; | ||
|
|
||
| @SuppressWarnings("checkstyle:parameternumber") | ||
| private BlockManagerParams(ExecutorServiceFuturePool futurePool, BlockData blockData, |
There was a problem hiding this comment.
we're still ending up with a constructor here. Instead of this, look at S3ClientFactory.S3ClientCreationParameters to see how we get and set parameters there, we should follow the same patter here.
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.hadoop.fs.impl.prefetch; |
There was a problem hiding this comment.
could you refactor this class, similar to S3ClientFactory.S3ClientCreationParameters. That feels like a cleaner way to get and set these parameters.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ahmarsuhail
requested changes
Nov 20, 2023
| return trackerFactory; | ||
| } | ||
|
|
||
| public BlockManagerParameters setFuturePool( |
There was a problem hiding this comment.
could you rename the setters to "withFuturePool" instead of set, keeps things uniform with other builders in the project.
There was a problem hiding this comment.
Also could you pelase add javadocs for the getters and setters
| @Test | ||
| public void testArgChecks() throws Exception { | ||
| MockS3ARemoteObject s3File = new MockS3ARemoteObject(FILE_SIZE, false); | ||
| S3ARemoteObjectReader reader = new S3ARemoteObjectReader(s3File); | ||
|
|
||
| Configuration conf = new Configuration(); | ||
| BlockManagerParameters blockManagerParamsBuilder1 = |
There was a problem hiding this comment.
with the builder, this test is getting really long now. How about we split it into separate tests? One test for the valid blockManager and the assertions on it that are done. And then split each of the illegal arg ones into a separate test? eg: testNullFuturePool(), etc.
| @@ -244,9 +292,18 @@ private void testPrefetchHelper(boolean forcePrefetchFailure) | |||
| throws IOException, InterruptedException { | |||
| MockS3ARemoteObject s3File = new MockS3ARemoteObject(FILE_SIZE, false); | |||
| S3ARemoteObjectReader reader = new S3ARemoteObjectReader(s3File); | |||
| BlockManagerParameters blockManagerParamsBuilder = | |||
There was a problem hiding this comment.
looks like the code to create blockManagerParamsBuilder is common in these three cases. Consider moving to a common method that returns the builder.
virajjasani
changed the title
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
added a commit
to virajjasani/hadoop
that referenced
this pull request
Nov 28, 2023
slfan1989
pushed a commit
that referenced
this pull request
Jan 21, 2024
…6302) Contributed by Viraj Jasani. Signed-off-by: Shilun Fan <slfan1989@apache.org>
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.
Jira: HADOOP-18959