YARN-11238. Optimizing FederationClientInterceptor Call with Parallelism. #4904
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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Sep 20, 2022
| // Generate parallel Callable tasks | ||
| for (SubClusterId subClusterId : subClusterIds) { | ||
| callables.add(() -> { | ||
| ApplicationClientProtocol protocol = |
There was a problem hiding this comment.
Thank you very much for your help reviewing the code, I will fix it.
| callables.add(() -> { | ||
| ApplicationClientProtocol protocol = | ||
| getClientRMProxyForSubCluster(subClusterId); | ||
| Method method = ApplicationClientProtocol.class |
| new Class[]{GetClusterNodesRequest.class}, new Object[]{request}); | ||
| Collection<GetClusterNodesResponse> clusterNodes = null; | ||
| try { | ||
| clusterNodes = invokeConcurrent(remoteMethod, GetClusterNodesResponse.class); |
There was a problem hiding this comment.
Can we define and return inside the try?
There was a problem hiding this comment.
I will refactor the code.
| Assert.assertTrue(clusterMetrics.isEmpty()); | ||
| Collection<GetClusterMetricsResponse> clusterMetrics = interceptor. | ||
| invokeConcurrent(remoteMethod, GetClusterMetricsResponse.class); | ||
| Assert.assertTrue(!clusterMetrics.isEmpty()); |
| Assert.assertTrue(clusterMetrics.isEmpty()); | ||
| Collection<GetClusterMetricsResponse> clusterMetrics = interceptor. | ||
| invokeConcurrent(remoteMethod, GetClusterMetricsResponse.class); | ||
| Assert.assertTrue(!clusterMetrics.isEmpty()); |
There was a problem hiding this comment.
Now the expectation is the opposite? What has changed?
There was a problem hiding this comment.
invokeConcurrent This function originally contains 3 parameters
- ArrayList clusterIds
- ClientMethod request
- Class clazz
The first parameter is the SubClusters currently active.
In order to get the first parameter, all calling functions need to call the following function
Map<SubClusterId, SubClusterInfo> subclusters = federationFacade.getSubClusters(true);
I think we can put the fetching of the 1st parameter inside invokeConcurrent, which is more reasonable.
This unit test tests a situation where there is no Subcluster available, so an empty result is returned.
Map<SubClusterId, GetClusterMetricsResponse> clusterMetrics = interceptor.
invokeConcurrent(new ArrayList<>(), remoteMethod, GetClusterMetricsResponse.class);
I removed the 1st parameter, it is a little difficult to simulate the situation where all Subclusters are unavailable.
I try to ensure that the original test logic can be completed, and I will refactor part of the code.
| * @throws YarnException if the call to get active subClusterIds is unsuccessful | ||
| */ | ||
| public Collection<SubClusterId> getActiveSubClusterIds() throws YarnException { | ||
| Map<SubClusterId, SubClusterInfo> activeSubClusters = |
| */ | ||
| public Collection<SubClusterId> getActiveSubClusterIds() throws YarnException { | ||
| Map<SubClusterId, SubClusterInfo> activeSubClusters = | ||
| getSubClusters(true); |
There was a problem hiding this comment.
Can we put the true in a variable to make it easier to understand what the true represents?
There was a problem hiding this comment.
Thanks for your suggestion, I will use the getSubClusters method. The way of using parameters (true) is better for code understanding.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Please help to review this pr again, thank you very much! |
|
@goiri Can you help review this pr? Thank you very much! |
goiri
approved these changes
Sep 21, 2022
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Sep 27, 2022
| throws YarnException { | ||
|
|
||
| // Get Active SubClusters | ||
| Map<SubClusterId, SubClusterInfo> subClusterInfo = |
There was a problem hiding this comment.
Thanks for your help reviewing the code, I will fix it.
| ClientMethod remoteMethod = new ClientMethod("getClusterMetrics", | ||
| new Class[] {GetClusterMetricsRequest.class}, | ||
| new Object[] {GetClusterMetricsRequest.newInstance()}); | ||
| Map<SubClusterId, GetClusterMetricsResponse> clusterMetrics = interceptor. | ||
| invokeConcurrent(new ArrayList<>(), remoteMethod, GetClusterMetricsResponse.class); | ||
| Collection<GetClusterMetricsResponse> clusterMetrics = interceptor. |
There was a problem hiding this comment.
Collection<GetClusterMetricsResponse> clusterMetrics = interceptor.invokeConcurrent(
remoteMethod, GetClusterMetricsResponse.class);
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Oct 4, 2022
|
@goiri Thank you very much for your help reviewing the code! |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
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: YARN-11238. Optimizing FederationClientInterceptor Call with Parallelism.