HDDS-1406. Avoid usage of commonPool in RatisPipelineUtils. #714
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
bharatviswa504
changed the title
|
💔 -1 overall
This message was automatically generated. |
lokeshj1703
reviewed
Apr 11, 2019
There was a problem hiding this comment.
I think we need to make submit call for each datanode.
lokeshj1703
reviewed
Apr 11, 2019
There was a problem hiding this comment.
We can use a simple executor as fork join pool is an executor for fork join tasks. Also we can always keep the number of threads to three as there is a network call involved in the task.
There was a problem hiding this comment.
Used ForkJoinPool can be used for normal tasks also. Refer NonFrok Join Clients.
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ForkJoinPool.html
Used this ForJoinPool here to take advantage of its WorkSteal Algorithm in allocating/claiming threads. I think this might be useful. More can be read from the documentation.
And if no of available processors is less than 3, even setting to 3 will not have any advantage. This is the reason for setting this.
There was a problem hiding this comment.
We can use three threads as these threads would ultimately be blocked on network IO and will not be CPU intensive. Also we need to shutdown the fork join pool.
bharatviswa504
force-pushed
the
HDDS-1406
branch
from
1a51c85 to
2cbbb60
Compare
|
/retest |
|
Thank You @arp7 for offline review comments. |
|
💔 -1 overall
This message was automatically generated. |
bharatviswa504
force-pushed
the
HDDS-1406
branch
from
2cbbb60 to
e667cf0
Compare
|
Thank You @lokeshj1703 for the review. |
bharatviswa504
force-pushed
the
HDDS-1406
branch
from
e667cf0 to
7300b49
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
arp7
reviewed
Apr 25, 2019
...op-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
shutdown will wait for previously submitted tasks to complete. You can probably call shutdownNow since we don't care about the completion on shutdown.
bharatviswa504
force-pushed
the
HDDS-1406
branch
from
7300b49 to
b877751
Compare
|
Thank You @arp7 for the review. |
arp7
reviewed
Apr 25, 2019
There was a problem hiding this comment.
Don't log here, just throw IOException that wraps the ExecutionException.
There was a problem hiding this comment.
Logging here, because in the actual createPipelines method in BackGroudPipelineCreator which calls this method when we throw IOException they break from while loop. That is the reason for the logging. Let me know if we still don't want to log here?
There was a problem hiding this comment.
That method is already logging right:
pipelineManager.createPipeline(type, factor);
} catch (IOException ioe) {
break;
} catch (Throwable t) {
LOG.error("Error while creating pipelines {}", t);
break;
}
So if we log here the exception will be logged twice. Perhaps I'm looking at it wrong.
There was a problem hiding this comment.
We return IOException, so in case of IOException we have no logging right?
It has only break;
There was a problem hiding this comment.
You can simplify the code a bit by catching multiple exceptions in one clause. e.g.
catch(ExecutionException|RejectedExecutionException e)
|
💔 -1 overall
This message was automatically generated. |
|
/retest |
|
💔 -1 overall
This message was automatically generated. |
lokeshj1703
reviewed
Apr 26, 2019
There was a problem hiding this comment.
Can you please check if one of the threads is not used up in waiting for parallel stream to finish execution? If it does then there are only two threads available for making a rpc call.
There was a problem hiding this comment.
Can we avoid making it static? The problem with static occurs in the MiniOzoneCluster tests. Once SCM is stopped by one of the tests, the fork join pool will be shutdown and will not be available again for execution. I think this might be a reason for unit test failures.
|
Test failures are not related to this patch. |
|
Thank You @lokeshj1703 for the review. |
|
💔 -1 overall
This message was automatically generated. |
lokeshj1703
reviewed
May 10, 2019
|
|
||
| PipelineFactory(NodeManager nodeManager, PipelineStateManager stateManager, | ||
| Configuration conf) { | ||
| Configuration conf, RatisPipelineUtils ratisPipelineUtils) { |
There was a problem hiding this comment.
@bharatviswa504 Thanks for adding the changes! An instance of RatisPipelineUtils does not seem right as it is a utility class. Could we move the functions of RatisPipelineUtils to RatisPipelineProvider instead?
|
Thank You @lokeshj1703 for the review. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
lokeshj1703
reviewed
May 16, 2019
| } | ||
|
|
||
| @VisibleForTesting | ||
| public PipelineProvider getProvider(ReplicationType type) { |
There was a problem hiding this comment.
We don't need to expose this api for now. In the test where it is used, we can call pipelineManager.createPipeline instead.
|
|
||
| void triggerPipelineCreation(); | ||
|
|
||
| PipelineFactory getPipelineFactory(); |
There was a problem hiding this comment.
We don't need to expose this api as well. The shutdown for pipelineFactory should be called from PipelineManager itself.
|
|
||
| @Override | ||
| public void shutdown() { | ||
| forkJoinPool.shutdownNow(); |
There was a problem hiding this comment.
We should also wait for the tasks to finish. We need to use awaitTermination call. We can use timeout of 60 seconds? That is what is used in Scheduler class.
There was a problem hiding this comment.
This is done based on arpit's comment, as on an unclean shutdown this terminate abruptly. So we can use shutdownNow(), instead of awaitTermination in normal case too.
There was a problem hiding this comment.
@bharatviswa504 I agree. We need to use shutdownNow but we also need to use awaitTermination. shutdownNow would interrupt the running tasks but the running task should handle the interrupt. If the task does not exit on interrupt, it is a better idea to wait for the task to finish.
| (pool -> { | ||
| final ForkJoinWorkerThread worker = ForkJoinPool. | ||
| defaultForkJoinWorkerThreadFactory.newThread(pool); | ||
| worker.setName("ratisCreatePipeline" + worker.getPoolIndex()); |
There was a problem hiding this comment.
NIT - "ratisCreatePipeline" - Can we make it all capital?
| * @param pipeline - Pipeline to be created | ||
| * @throws IOException if creation fails | ||
| */ | ||
| public void createPipeline(Pipeline pipeline) |
There was a problem hiding this comment.
We can remove this fn and replace it with initializePipeline.
| } | ||
|
|
||
| @Override | ||
| public PipelineFactory getPipelineFactory() { |
There was a problem hiding this comment.
We dont need to expose this api. We can add a shutdown call for the pipelineFactory in the SCMPipelineManager#close fn.
|
|
||
| @Override | ||
| public void shutdown() { | ||
|
|
There was a problem hiding this comment.
NIT - Can we add a comment like //do nothing.
| } | ||
|
|
||
| // shutdown pipeline provider. | ||
| pipelineManager.getPipelineFactory().shutdown(); |
There was a problem hiding this comment.
We can remove this call.
|
|
||
| @Override | ||
| public void shutdown() { | ||
|
|
There was a problem hiding this comment.
NIT - Can we add a comment like // do nothing?
| HddsProtos.ReplicationType.RATIS); | ||
| try { | ||
| RatisPipelineUtils.createPipeline(pipelines.get(0), conf); | ||
| ratisPipelineProvider.createPipeline(pipelines.get(0)); |
There was a problem hiding this comment.
We can use pipelineManager#create call instead.
|
Thank You @lokeshj1703 for the review. |
lokeshj1703
reviewed
May 20, 2019
| private final Configuration conf; | ||
|
|
||
| // Set parallelism at 3, as now in Ratis we create 1 and 3 node pipelines. | ||
| private final int parallelisimForPool = 3; |
There was a problem hiding this comment.
There is a typo. parallelis'i'mForPool.
|
@lokeshj1703 Addressed the review comments. |
|
💔 -1 overall
This message was automatically generated. |
|
@bharatviswa504 Thanks for updating the PR! The changes look good to me. +1. |
|
@bharatviswa504 can you check if the UT failure is related to the patch? |
|
Failed test is not related to this patch. I think we can improve waitForClusterToBeReady to also check for safeMode also, so that we don't see these failures randomly. (Not completely sure we need though, if it is needed, I can open a new Jira to fix this.) |
|
Thank You @lokeshj1703 and @arp7 for the review. |
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
Author: Sanil15 <[email protected]> Reviewers: Jagadish<[email protected]> Closes apache#714 from Sanil15/SAMZA-1930
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
No description provided.