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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
