YARN-11238. Optimizing FederationClientInterceptor Call with Parallelism. #4904

slfan1989 · 2022-09-18T12:47:35Z

hadoop-yetus · 2022-09-18T14:26:16Z

hadoop-yetus · 2022-09-19T00:09:11Z

hadoop-yetus · 2022-09-20T08:49:16Z

hadoop-yetus · 2022-09-20T09:30:40Z

hadoop-yetus · 2022-09-20T11:14:31Z

hadoop-yetus · 2022-09-20T13:04:08Z

goiri · 2022-09-20T16:42:15Z

...src/main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java

+    // Generate parallel Callable tasks
+    for (SubClusterId subClusterId : subClusterIds) {
+      callables.add(() -> {
+        ApplicationClientProtocol protocol =


Single line.

Thank you very much for your help reviewing the code, I will fix it.

goiri · 2022-09-20T16:42:37Z

...src/main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java

+      callables.add(() -> {
+        ApplicationClientProtocol protocol =
+            getClientRMProxyForSubCluster(subClusterId);
+        Method method = ApplicationClientProtocol.class


Single line.

I will fix it.

goiri · 2022-09-20T16:43:50Z

...src/main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java

+        new Class[]{GetClusterNodesRequest.class}, new Object[]{request});
+    Collection<GetClusterNodesResponse> clusterNodes = null;
+    try {
+      clusterNodes = invokeConcurrent(remoteMethod, GetClusterNodesResponse.class);


Can we define and return inside the try?

I will refactor the code.

goiri · 2022-09-20T16:44:51Z

...test/java/org/apache/hadoop/yarn/server/router/clientrm/TestFederationClientInterceptor.java

-    Assert.assertTrue(clusterMetrics.isEmpty());
+    Collection<GetClusterMetricsResponse> clusterMetrics = interceptor.
+        invokeConcurrent(remoteMethod, GetClusterMetricsResponse.class);
+    Assert.assertTrue(!clusterMetrics.isEmpty());


assertFalse

I will fix it.

goiri · 2022-09-20T16:45:06Z

...test/java/org/apache/hadoop/yarn/server/router/clientrm/TestFederationClientInterceptor.java

-    Assert.assertTrue(clusterMetrics.isEmpty());
+    Collection<GetClusterMetricsResponse> clusterMetrics = interceptor.
+        invokeConcurrent(remoteMethod, GetClusterMetricsResponse.class);
+    Assert.assertTrue(!clusterMetrics.isEmpty());


Now the expectation is the opposite? What has changed?

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.

goiri · 2022-09-20T16:45:19Z

...src/main/java/org/apache/hadoop/yarn/server/federation/utils/FederationStateStoreFacade.java

+   * @throws YarnException if the call to get active subClusterIds is unsuccessful
+   */
+  public Collection<SubClusterId> getActiveSubClusterIds() throws YarnException {
+    Map<SubClusterId, SubClusterInfo> activeSubClusters =


Single line.

I will fix it.

goiri · 2022-09-20T16:46:16Z

...src/main/java/org/apache/hadoop/yarn/server/federation/utils/FederationStateStoreFacade.java

+   */
+  public Collection<SubClusterId> getActiveSubClusterIds() throws YarnException {
+    Map<SubClusterId, SubClusterInfo> activeSubClusters =
+        getSubClusters(true);


Can we put the true in a variable to make it easier to understand what the true represents?

Thanks for your suggestion, I will use the getSubClusters method. The way of using parameters (true) is better for code understanding.

hadoop-yetus · 2022-09-20T16:53:36Z

hadoop-yetus · 2022-09-21T13:12:36Z

slfan1989 · 2022-09-21T15:57:31Z

slfan1989 · 2022-09-21T15:58:12Z

hadoop-yetus · 2022-09-22T23:56:56Z

hadoop-yetus · 2022-09-27T00:49:23Z

goiri · 2022-09-27T20:27:06Z

...src/main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java

+      throws YarnException {
+
+    // Get Active SubClusters
+    Map<SubClusterId, SubClusterInfo> subClusterInfo =


Thanks for your help reviewing the code, I will fix it.

goiri · 2022-09-27T20:28:14Z

...test/java/org/apache/hadoop/yarn/server/router/clientrm/TestFederationClientInterceptor.java

    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.


Collection<GetClusterMetricsResponse> clusterMetrics = interceptor.invokeConcurrent( remoteMethod, GetClusterMetricsResponse.class);

I will fix it.

hadoop-yetus · 2022-09-29T18:12:25Z

hadoop-yetus · 2022-10-01T13:51:39Z

slfan1989 · 2022-10-04T22:59:45Z

YARN-11238. Optimizing FederationClientInterceptor Call with Parallel…

2dcf58f

…ism.

slfan1989 marked this pull request as draft September 18, 2022 12:47

YARN-11238. Add getActiveSubClusterIds.

0ac9aa7

slfan1989 and others added 2 commits September 20, 2022 14:40

Merge branch 'trunk' into YARN-11238

b2db6e1

YARN-11308. Fix CodeStyle.

d0919f6

YARN-11238. Fix CodeStyle.

7cbca99

YARN-11238. Fix CheckStyle.

47ff19b

YARN-11238. Fix CheckStyle.

5b63e15

slfan1989 marked this pull request as ready for review September 20, 2022 14:47

goiri reviewed Sep 20, 2022

View reviewed changes

YARN-11238. Fix CheckStyle.

29ac70f

slfan1989 requested a review from goiri September 21, 2022 15:57

goiri approved these changes Sep 21, 2022

View reviewed changes

slfan1989 and others added 3 commits September 22, 2022 22:43

YARN-11238. Fix CheckStyle.

cf40ba0

Merge branch 'apache:trunk' into YARN-11238

8c2bd85

YARN-11238. Fix CheckStyle.

285b6cb

Merge branch 'apache:trunk' into YARN-11238

697eed7

goiri approved these changes Sep 27, 2022

View reviewed changes

YARN-11238. Fix CheckStyle.

d68ac31

Merge branch 'apache:trunk' into YARN-11238

6ff6dd8

goiri approved these changes Oct 4, 2022

View reviewed changes

goiri merged commit 22bd5e3 into apache:trunk Oct 4, 2022

HarshitGupta11 pushed a commit to HarshitGupta11/hadoop that referenced this pull request Nov 28, 2022

YARN-11238. Optimizing FederationClientInterceptor Call with Parallel…

4edbfcf

…ism. (apache#4904)

YARN-11238. Optimizing FederationClientInterceptor Call with Parallelism. #4904

YARN-11238. Optimizing FederationClientInterceptor Call with Parallelism. #4904

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!