goiri commented on code in PR #4846:
URL: https://github.com/apache/hadoop/pull/4846#discussion_r964122414
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##########
@@ -255,24 +261,52 @@ public GetApplicationHomeSubClusterResponse
getApplicationHomeSubCluster(
@Override
public GetApplicationsHomeSubClusterResponse getApplicationsHomeSubCluster(
GetApplicationsHomeSubClusterRequest request) throws YarnException {
- long start = clock.getTime();
- List<ApplicationHomeSubCluster> result = new ArrayList<>();
+
+ if (request == null) {
+ throw new YarnException("Missing getApplicationsHomeSubCluster request");
+ }
try {
- for (String child : zkManager.getChildren(appsZNode)) {
- ApplicationId appId = ApplicationId.fromString(child);
- SubClusterId homeSubCluster = getApp(appId);
- ApplicationHomeSubCluster app =
- ApplicationHomeSubCluster.newInstance(appId, homeSubCluster);
- result.add(app);
- }
+ long start = clock.getTime();
+ SubClusterId requestSC = request.getSubClusterId();
+ List<String> children = zkManager.getChildren(appsZNode);
+ List<ApplicationHomeSubCluster> result =
+ children.stream().map(child -> generateAppHomeSC(child))
+ .filter(appHomeSC -> judgeAdd(requestSC,
appHomeSC.getHomeSubCluster()))
+ .limit(maxAppsInStateStore)
+ .collect(Collectors.toList());
+ long end = clock.getTime();
+ opDurations.addGetAppsHomeSubClusterDuration(start, end);
+ LOG.info("filterSubClusterId = {}, appCount = {}.", requestSC,
result.size());
+ return GetApplicationsHomeSubClusterResponse.newInstance(result);
} catch (Exception e) {
String errMsg = "Cannot get apps: " + e.getMessage();
FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg);
}
- long end = clock.getTime();
- opDurations.addGetAppsHomeSubClusterDuration(start, end);
- return GetApplicationsHomeSubClusterResponse.newInstance(result);
+
+ throw new YarnException("Cannot get app by request");
+ }
+
+ private ApplicationHomeSubCluster generateAppHomeSC(String appId) {
+ try {
+ ApplicationId applicationId = ApplicationId.fromString(appId);
+ SubClusterId homeSubCluster = getApp(applicationId);
+ ApplicationHomeSubCluster app =
+ ApplicationHomeSubCluster.newInstance(applicationId, homeSubCluster);
+ return app;
+ } catch (Exception ex) {
+ LOG.error("get homeSubCluster by appId = {}.", appId);
+ }
+ return null;
+ }
+
+ private boolean judgeAdd(SubClusterId filterSubCluster, SubClusterId
homeSubCluster) {
Review Comment:
`judgeAdd` is a weird name fot this function.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/test/java/org/apache/hadoop/yarn/server/federation/store/impl/FederationStateStoreBaseTest.java:
##########
@@ -410,6 +412,89 @@ public void testGetApplicationsHomeSubCluster() throws
Exception {
Assert.assertTrue(result.getAppsHomeSubClusters().contains(ahsc2));
}
+ @Test
+ public void testGetApplicationsHomeSubClusterEmpty() throws Exception {
+ LambdaTestUtils.intercept(YarnException.class,
+ "Missing getApplicationsHomeSubCluster request",
+ () -> stateStore.getApplicationsHomeSubCluster(null));
+ }
+
+ @Test
+ public void testGetApplicationsHomeSubClusterFilter() throws Exception {
+ // Add ApplicationHomeSC - SC1
+ long now = Time.now();
+
+ Set<ApplicationHomeSubCluster> appHomeSubClusters = new HashSet<>();
+
+ for (int i = 0; i < 10; i++) {
Review Comment:
Make 10 and 20 constants.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/test/java/org/apache/hadoop/yarn/server/router/clientrm/TestableFederationClientInterceptor.java:
##########
@@ -90,6 +95,7 @@ protected ApplicationClientProtocol
getClientRMProxyForSubCluster(
mockRMs.put(subClusterId, mockRM);
}
initNodeAttributes(subClusterId, mockRM);
+ initReservationSystem(mockRM);
Review Comment:
This seems out of scope.
Should we do it in a separate PR?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/MemoryFederationStateStore.java:
##########
@@ -255,17 +261,36 @@ public GetApplicationHomeSubClusterResponse
getApplicationHomeSubCluster(
@Override
public GetApplicationsHomeSubClusterResponse getApplicationsHomeSubCluster(
GetApplicationsHomeSubClusterRequest request) throws YarnException {
- List<ApplicationHomeSubCluster> result =
- new ArrayList<ApplicationHomeSubCluster>();
- for (Entry<ApplicationId, SubClusterId> e : applications.entrySet()) {
- result
- .add(ApplicationHomeSubCluster.newInstance(e.getKey(),
e.getValue()));
+
+ if (request == null) {
+ throw new YarnException("Missing getApplicationsHomeSubCluster request");
}
- GetApplicationsHomeSubClusterResponse.newInstance(result);
+ SubClusterId requestSC = request.getSubClusterId();
+ List<ApplicationHomeSubCluster> result = applications.keySet().stream()
+ .map(applicationId -> generateAppHomeSC(applicationId))
+ .filter(appHomeSC -> judgeAdd(requestSC,
appHomeSC.getHomeSubCluster()))
+ .limit(maxAppsInStateStore)
+ .collect(Collectors.toList());
+
+ LOG.info("filterSubClusterId = {}, appCount = {}.", requestSC,
result.size());
return GetApplicationsHomeSubClusterResponse.newInstance(result);
}
+ private ApplicationHomeSubCluster generateAppHomeSC(ApplicationId
applicationId) {
+ SubClusterId subClusterId = applications.get(applicationId);
+ return ApplicationHomeSubCluster.newInstance(applicationId, subClusterId);
+ }
+
+ private boolean judgeAdd(SubClusterId filterSubCluster, SubClusterId
homeSubCluster) {
+ if (filterSubCluster == null) {
+ return true;
+ } else if (filterSubCluster.equals(homeSubCluster)) {
Review Comment:
No need for else
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java:
##########
@@ -949,7 +949,7 @@ public ReservationSubmissionResponse submitReservation(
// Second, determine whether the current ReservationId has a
corresponding subCluster.
// If it does not exist, add it. If it exists, update it.
Boolean exists = existsReservationHomeSubCluster(reservationId);
- if (!exists) {
+ if (!exists || i == 0) {
Review Comment:
What is this for?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/records/GetApplicationsHomeSubClusterRequest.java:
##########
@@ -37,4 +38,33 @@ public static GetApplicationsHomeSubClusterRequest
newInstance() {
return request;
}
+ @Private
+ @Unstable
+ public static GetApplicationsHomeSubClusterRequest
+ newInstance(SubClusterId subClusterId) {
+ GetApplicationsHomeSubClusterRequest request =
+ Records.newRecord(GetApplicationsHomeSubClusterRequest.class);
+ request.setSubClusterId(subClusterId);
+ return request;
+ }
+
+ /**
+ * Get the {@link SubClusterId} representing the unique identifier of the
+ * subcluster.
+ *
+ * @return the subcluster identifier
+ */
+ @InterfaceAudience.Public
Review Comment:
Use @Public
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
