[
https://issues.apache.org/jira/browse/YARN-11290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17600963#comment-17600963
]
ASF GitHub Bot commented on YARN-11290:
---------------------------------------
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
> Improve Query Condition of FederationStateStore#getApplicationsHomeSubCluster
> -----------------------------------------------------------------------------
>
> Key: YARN-11290
> URL: https://issues.apache.org/jira/browse/YARN-11290
> Project: Hadoop YARN
> Issue Type: Improvement
> Components: federation
> Affects Versions: 3.4.0
> Reporter: fanshilun
> Assignee: fanshilun
> Priority: Minor
> Labels: pull-request-available
> Attachments: SqlServer-StoredProc.png
>
>
> 1. Currently this interface returns the number of apps in all sub-clusters,
> increasing the limit on the number of query apps, limited to 1000 apps.
> 2. Allows to query the App based on the specified HomeSubCluster.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
