Hean-Chhinling commented on code in PR #8033:
URL: https://github.com/apache/hadoop/pull/8033#discussion_r2440174526
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/dao/NodeLabelsInfo.java:
##########
@@ -48,7 +48,7 @@ public NodeLabelsInfo(List<NodeLabel> nodeLabels) {
this.nodeLabelsInfo.add(new NodeLabelInfo(label));
}
}
-
+
Review Comment:
Nit: look like this an unintentional space.
Could we clean this up?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/helper/AppInfoJsonVerifications.java:
##########
@@ -88,8 +87,8 @@ public static void verify(JSONObject info, RMApp app) throws
JSONException {
"clusterUsagePerc doesn't match");
assertEquals(1, info.getInt("runningContainers"),
"numContainers doesn't match");
- assertNotNull(info.get("preemptedResourceSecondsMap"),
- "preemptedResourceSecondsMap should not be null");
+ assertTrue(info.isNull("preemptedResourceSecondsMap"),
+ "preemptedResourceSecondsMap should be null, cause it is empty");
Review Comment:
Would you mind help giving some context here?
I could not find the preemptedResourceSecondsMap that it should be empty
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/TestRMWebServicesApps.java:
##########
@@ -564,9 +555,9 @@ public void testAppsQueryFinalStatusNone() throws
JSONException, Exception {
.request(MediaType.APPLICATION_JSON).get(Response.class);
assertEquals(MediaType.APPLICATION_JSON_TYPE + ";" + JettyUtils.UTF_8,
response.getMediaType().toString());
- JSONObject json = response.readEntity(JSONObject.class);
+ JSONObject json = responseToJson(response);
assertEquals(1, json.length(), "incorrect number of elements");
- assertEquals("", json.get("apps").toString(), "apps is not null");
+ assertEquals(new JSONObject().toString(), json.get("apps").toString(),
"apps is not null");
Review Comment:
What do you think of using an empty array `{}` instead of initializing an
object `new JSONObject().toString()` just to get the empty array?
I tested it like the followings and it still pass, but please let me know
what do you think :)
`assertEquals("{}", json.get("apps").toString(), "apps is not null");`
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/federation/TestFederationRMStateStoreService.java:
##########
@@ -199,10 +190,10 @@ private void explicitFailover(MockRM rm) throws
IOException {
stateStore = rm.getFederationStateStoreService().getStateStoreClient();
}
- private void checkClusterMetricsInfo(String capability, int numNodes)
- throws JAXBException {
- ClusterMetricsInfo clusterMetricsInfo = jsonUnmarshaller.unmarshalFromJSON(
- new StringReader(capability), ClusterMetricsInfo.class);
+ private void checkClusterMetricsInfo(String capability, int numNodes) {
+ ClusterMetricsInfo clusterMetricsInfo = fromJson(capability,
ClusterMetricsInfo.class);
+ System.err.println(capability);
+ System.err.println(clusterMetricsInfo.getTotalNodes());
Review Comment:
Is the System.err.println() needed for long term as well?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/dao/NodeLabelsInfo.java:
##########
@@ -75,7 +75,7 @@ public Set<NodeLabel> getNodeLabels() {
}
return nodeLabels;
}
-
+
Review Comment:
Nit: Here as well, unintentional space.
--
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]
