goiri commented on code in PR #5420:
URL: https://github.com/apache/hadoop/pull/5420#discussion_r1142378381
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/utils/FederationStateStoreFacade.java:
##########
@@ -968,7 +969,7 @@ public int getActiveSubClustersCount() throws YarnException
{
* @throws YarnException When there is no Active SubCluster,
* an exception will be thrown (No active SubCluster available to submit the
request.)
*/
- public static SubClusterId getRandomActiveSubCluster(
+ public SubClusterId getRandomActiveSubCluster(
Review Comment:
Isn't this still static?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/FederationInterceptorREST.java:
##########
@@ -586,37 +583,31 @@ private Response
invokeSubmitApplication(ApplicationSubmissionContextInfo submis
* operation.
*/
@Override
- public AppInfo getApp(HttpServletRequest hsr, String appId,
- Set<String> unselectedFields) {
+ public AppInfo getApp(HttpServletRequest hsr, String appId, Set<String>
unselectedFields) {
long startTime = clock.getTime();
- ApplicationId applicationId = null;
try {
- applicationId = ApplicationId.fromString(appId);
+ ApplicationId.fromString(appId);
Review Comment:
Why do we do this if we don't use it?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/FederationInterceptorREST.java:
##########
@@ -2318,7 +2220,7 @@ public Response createNewReservation(HttpServletRequest
hsr)
private Response invokeCreateNewReservation(Map<SubClusterId,
SubClusterInfo> subClustersActive,
List<SubClusterId> blackList, HttpServletRequest hsr, int retryCount)
- throws YarnException, IOException, InterruptedException {
+ throws YarnException {
SubClusterId subClusterId =
federationFacade.getRandomActiveSubCluster(subClustersActive,
blackList);
Review Comment:
Just make this call static.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/FederationInterceptorREST.java:
##########
@@ -1451,7 +1361,7 @@ public AppActivitiesInfo
getAppActivities(HttpServletRequest hsr,
long startTime = clock.getTime();
SubClusterInfo subClusterInfo = getHomeSubClusterInfoByAppId(appId);
Review Comment:
Create one with appId and then you do the proper check for null, etc.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/test/java/org/apache/hadoop/yarn/server/router/webapp/TestFederationInterceptorREST.java:
##########
@@ -641,7 +632,7 @@ public void testGetApplicationState()
*/
@Test
public void testGetApplicationStateNotExists()
Review Comment:
One line?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/FederationInterceptorREST.java:
##########
@@ -1108,29 +1030,20 @@ public ClusterMetricsInfo call() {
public AppState getAppState(HttpServletRequest hsr, String appId)
throws AuthorizationException {
- ApplicationId applicationId = null;
- try {
- applicationId = ApplicationId.fromString(appId);
- } catch (IllegalArgumentException e) {
- return null;
- }
+ // Check that the appId format is accurate
+ RouterServerUtil.validateApplicationId(appId);
- SubClusterInfo subClusterInfo = null;
- SubClusterId subClusterId = null;
+ // Get SubCluster according to appId.
+ SubClusterInfo subClusterInfo;
try {
- subClusterId =
- federationFacade.getApplicationHomeSubCluster(applicationId);
- if (subClusterId == null) {
- return null;
- }
- subClusterInfo = federationFacade.getSubCluster(subClusterId);
+ subClusterInfo = getHomeSubClusterInfoByAppId(appId);
} catch (YarnException e) {
+ LOG.error("getHomeSubClusterInfoByAppId error, applicationId = {}.",
appId, e);
return null;
}
- DefaultRequestInterceptorREST interceptor =
- getOrCreateInterceptorForSubCluster(subClusterId,
- subClusterInfo.getRMWebServiceAddress());
+ // Call the appState interface.
+ DefaultRequestInterceptorREST interceptor =
getOrCreateInterceptorForSubCluster(subClusterInfo);
Review Comment:
As you are consolidating, you could create methods with appId and nodeId.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/test/java/org/apache/hadoop/yarn/server/router/webapp/TestFederationInterceptorREST.java:
##########
@@ -1262,6 +1250,7 @@ public void testSubmitReservation() throws Exception {
Assert.assertNotNull(entity);
Assert.assertNotNull(entity instanceof ReservationListInfo);
+ assert entity instanceof ReservationListInfo;
Review Comment:
Do a junit Assert.assertTrue()
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/utils/FederationStateStoreFacade.java:
##########
@@ -968,7 +969,7 @@ public int getActiveSubClustersCount() throws YarnException
{
* @throws YarnException When there is no Active SubCluster,
* an exception will be thrown (No active SubCluster available to submit the
request.)
*/
- public static SubClusterId getRandomActiveSubCluster(
+ public SubClusterId getRandomActiveSubCluster(
Review Comment:
We should change the places where we call it to call it as static.
--
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]
