You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by "goiri (via GitHub)" <gi...@apache.org> on 2023/05/08 17:12:27 UTC
[GitHub] [hadoop] goiri commented on a diff in pull request #5631: YARN-11479. [Federation] ZookeeperFederationStateStore Support Store ApplicationSubmitData.
goiri commented on code in PR #5631:
URL: https://github.com/apache/hadoop/pull/5631#discussion_r1187678833
##########
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:
##########
@@ -325,47 +328,53 @@ public AddApplicationHomeSubClusterResponse addApplicationHomeSubCluster(
FederationApplicationHomeSubClusterStoreInputValidator.validate(request);
ApplicationHomeSubCluster app = request.getApplicationHomeSubCluster();
Review Comment:
Not very clear name to call it `app`.
##########
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:
##########
@@ -379,14 +388,27 @@ public GetApplicationHomeSubClusterResponse getApplicationHomeSubCluster(
long start = clock.getTime();
FederationApplicationHomeSubClusterStoreInputValidator.validate(request);
ApplicationId appId = request.getApplicationId();
- SubClusterId homeSubCluster = getApp(appId);
- if (homeSubCluster == null) {
+ ApplicationHomeSubCluster appHomeSubCluster = getApp(appId);
+ if (appHomeSubCluster == null) {
String errMsg = "Application " + appId + " does not exist";
FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg);
}
+
+ // Whether the returned result contains context
+ ApplicationSubmissionContext submissionContext =
+ appHomeSubCluster.getApplicationSubmissionContext();
+ boolean containsAppSubmissionContext = request.getContainsAppSubmissionContext();
Review Comment:
Why do we need the contains? Wouldn't it just return null?
If needed I would go in proper order:
```
if (request.getContainsAppSubmissionContext()) {
ApplicationSubmissionContext submissionContext =
appHomeSubCluster.getApplicationSubmissionContext();
return GetApplicationHomeSubClusterResponse.newInstance(appId, homeSubCluster, createTime,
submissionContext);
}
```
##########
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:
##########
@@ -325,47 +328,53 @@ public AddApplicationHomeSubClusterResponse addApplicationHomeSubCluster(
FederationApplicationHomeSubClusterStoreInputValidator.validate(request);
ApplicationHomeSubCluster app = request.getApplicationHomeSubCluster();
ApplicationId appId = app.getApplicationId();
+ ApplicationSubmissionContext appSubmissionContext = app.getApplicationSubmissionContext();
+
+ LOG.info("appId = {}, homeSubClusterId = {}, appSubmissionContext = {}.", appId, app,
+ appSubmissionContext);
// Try to write the subcluster
SubClusterId homeSubCluster = app.getHomeSubCluster();
try {
- putApp(appId, homeSubCluster, false);
+ putApp(appId, app, false);
} catch (Exception e) {
String errMsg = "Cannot add application home subcluster for " + appId;
FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg);
}
// Check for the actual subcluster
try {
- homeSubCluster = getApp(appId);
+ ApplicationHomeSubCluster appHomeSubCluster = getApp(appId);
Review Comment:
Getting this now is pretty confusing.
##########
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:
##########
@@ -379,14 +388,27 @@ public GetApplicationHomeSubClusterResponse getApplicationHomeSubCluster(
long start = clock.getTime();
FederationApplicationHomeSubClusterStoreInputValidator.validate(request);
ApplicationId appId = request.getApplicationId();
- SubClusterId homeSubCluster = getApp(appId);
- if (homeSubCluster == null) {
+ ApplicationHomeSubCluster appHomeSubCluster = getApp(appId);
+ if (appHomeSubCluster == null) {
String errMsg = "Application " + appId + " does not exist";
FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg);
}
+
+ // Whether the returned result contains context
+ ApplicationSubmissionContext submissionContext =
+ appHomeSubCluster.getApplicationSubmissionContext();
+ boolean containsAppSubmissionContext = request.getContainsAppSubmissionContext();
+ long creatTime = appHomeSubCluster.getCreateTime();
Review Comment:
`createTime`
And add the units.
##########
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:
##########
@@ -325,47 +328,53 @@ public AddApplicationHomeSubClusterResponse addApplicationHomeSubCluster(
FederationApplicationHomeSubClusterStoreInputValidator.validate(request);
ApplicationHomeSubCluster app = request.getApplicationHomeSubCluster();
ApplicationId appId = app.getApplicationId();
+ ApplicationSubmissionContext appSubmissionContext = app.getApplicationSubmissionContext();
+
+ LOG.info("appId = {}, homeSubClusterId = {}, appSubmissionContext = {}.", appId, app,
+ appSubmissionContext);
// Try to write the subcluster
SubClusterId homeSubCluster = app.getHomeSubCluster();
Review Comment:
What do we use this for now?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/test/java/org/apache/hadoop/yarn/server/federation/store/impl/TestZookeeperFederationStateStore.java:
##########
@@ -276,4 +283,30 @@ protected void checkRouterStoreToken(RMDelegationTokenIdentifier identifier,
assertNotNull(zkRouterStoreToken);
assertEquals(token, zkRouterStoreToken);
}
+
+ @Test
+ public void testGetApplicationHomeSubClusterWithContext() throws Exception {
+ ZookeeperFederationStateStore zkFederationStateStore =
+ (ZookeeperFederationStateStore) this.getStateStore();
+
+ ApplicationId appId = ApplicationId.newInstance(1, 3);
+ SubClusterId subClusterId = SubClusterId.newInstance("SC");
+ ApplicationSubmissionContext context =
+ ApplicationSubmissionContext.newInstance(appId, "test", "default",
+ Priority.newInstance(0), null, true, true,
+ 2, Resource.newInstance(10, 2), "test");
+ addApplicationHomeSC(appId, subClusterId, context);
+
+ GetApplicationHomeSubClusterRequest getRequest =
+ GetApplicationHomeSubClusterRequest.newInstance(appId, true);
+ GetApplicationHomeSubClusterResponse result =
+ zkFederationStateStore.getApplicationHomeSubCluster(getRequest);
+
+ assertEquals(appId,
+ result.getApplicationHomeSubCluster().getApplicationId());
Review Comment:
Extract `result.getApplicationHomeSubCluster()`.
--
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-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org