brumi1024 commented on a change in pull request #3763:
URL: https://github.com/apache/hadoop/pull/3763#discussion_r782982871
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAHSv2ClientImpl.java
##########
@@ -107,6 +113,91 @@ public void testGetAppAttemptReport() throws IOException,
YarnException {
isEqualTo("test original tracking url");
}
+ @Test
+ public void testGetContainerByAppAttempt() throws IOException, YarnException
{
+ // Single container tests
+ final ApplicationId appId1 = ApplicationId.newInstance(0, 1);
+ final ApplicationId appId2 = ApplicationId.newInstance(0, 2);
Review comment:
Is the second application necessary? It tests the same thing, just with
a different ID.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAHSv2ClientImpl.java
##########
@@ -107,6 +113,91 @@ public void testGetAppAttemptReport() throws IOException,
YarnException {
isEqualTo("test original tracking url");
}
+ @Test
+ public void testGetContainerByAppAttempt() throws IOException, YarnException
{
+ // Single container tests
+ final ApplicationId appId1 = ApplicationId.newInstance(0, 1);
+ final ApplicationId appId2 = ApplicationId.newInstance(0, 2);
+
+ final ApplicationAttemptId appAttemptId1 =
Review comment:
While using finals in local variables isn't a hindrance in Java, I see
little benefit here and generally the code doesn't use it, so I would prefer
not to use it in this method.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAHSv2ClientImpl.java
##########
@@ -107,6 +113,91 @@ public void testGetAppAttemptReport() throws IOException,
YarnException {
isEqualTo("test original tracking url");
}
+ @Test
+ public void testGetContainerByAppAttempt() throws IOException, YarnException
{
+ // Single container tests
+ final ApplicationId appId1 = ApplicationId.newInstance(0, 1);
+ final ApplicationId appId2 = ApplicationId.newInstance(0, 2);
+
+ final ApplicationAttemptId appAttemptId1 =
+ ApplicationAttemptId.newInstance(appId1, 1);
+ final ApplicationAttemptId appAttemptId2 =
+ ApplicationAttemptId.newInstance(appId2, 2);
+
+ final ContainerId containerId1 = ContainerId.newContainerId(appAttemptId1,
1);
+ final ContainerId containerId2 = ContainerId.newContainerId(appAttemptId2,
2);
+
+ // Multiple container test
+ final int multipleContainerSize = 4;
+ final int appMultipleId = 3;
+ final int appAttemptMultipleId = 3;
+ final ApplicationId appMultiple = ApplicationId.newInstance(0,
appMultipleId);
+ final ApplicationAttemptId appAttemptIdMultiple =
+ ApplicationAttemptId.newInstance(appMultiple,
appAttemptMultipleId);
+
+ List<TimelineEntity> containerEntities = new ArrayList<>();
+ for (int newEntityIdx = 0; newEntityIdx < multipleContainerSize;
++newEntityIdx) {
+ containerEntities.add(createContainerEntity(ContainerId.newContainerId(
+ appAttemptIdMultiple, containerId2.getContainerId() +1 +
newEntityIdx)));
+ }
Review comment:
To avoid offsetting the IDs (like the +3 below in
assertThat(report.getContainerId().getContainerId()).isEqualTo(containerIdx +
3);) could you please move the multiple container case to another test method?
Because currently if someone wants to add a new app+attempt+container he/she
needs to edit the offset below which isn't trivial at first.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAHSv2ClientImpl.java
##########
@@ -107,6 +113,91 @@ public void testGetAppAttemptReport() throws IOException,
YarnException {
isEqualTo("test original tracking url");
}
+ @Test
+ public void testGetContainerByAppAttempt() throws IOException, YarnException
{
+ // Single container tests
+ final ApplicationId appId1 = ApplicationId.newInstance(0, 1);
+ final ApplicationId appId2 = ApplicationId.newInstance(0, 2);
+
+ final ApplicationAttemptId appAttemptId1 =
+ ApplicationAttemptId.newInstance(appId1, 1);
+ final ApplicationAttemptId appAttemptId2 =
+ ApplicationAttemptId.newInstance(appId2, 2);
+
+ final ContainerId containerId1 = ContainerId.newContainerId(appAttemptId1,
1);
+ final ContainerId containerId2 = ContainerId.newContainerId(appAttemptId2,
2);
+
+ // Multiple container test
+ final int multipleContainerSize = 4;
+ final int appMultipleId = 3;
+ final int appAttemptMultipleId = 3;
+ final ApplicationId appMultiple = ApplicationId.newInstance(0,
appMultipleId);
+ final ApplicationAttemptId appAttemptIdMultiple =
+ ApplicationAttemptId.newInstance(appMultiple,
appAttemptMultipleId);
+
+ List<TimelineEntity> containerEntities = new ArrayList<>();
+ for (int newEntityIdx = 0; newEntityIdx < multipleContainerSize;
++newEntityIdx) {
+ containerEntities.add(createContainerEntity(ContainerId.newContainerId(
+ appAttemptIdMultiple, containerId2.getContainerId() +1 +
newEntityIdx)));
+ }
+
+ when(spyTimelineReaderClient.getContainerEntities(
+ appId1, "ALL",
+ ImmutableMap.<String, String>builder()
+ .put("appattemptId", appAttemptId1.toString())
+ .build(),
+ 0, null))
+ .thenReturn(Arrays.asList(createContainerEntity(containerId1)));
+ when(spyTimelineReaderClient.getContainerEntities(
+ appId2, "ALL",
+ ImmutableMap.<String, String>builder()
+ .put("appattemptId", appAttemptId2.toString())
+ .build(),
+ 0, null))
+ .thenReturn(Arrays.asList(createContainerEntity(containerId2)));
+
+ when(spyTimelineReaderClient.getContainerEntities(
+ appMultiple, "ALL",
+ ImmutableMap.<String, String>builder()
+ .put("appattemptId", appAttemptIdMultiple.toString())
+ .build(),
+ 0, null))
+ .thenReturn(containerEntities);
+
+ when(spyTimelineReaderClient.getApplicationEntity(appId1, "ALL", null))
+ .thenReturn(createApplicationTimelineEntity(appId1, true,
+ false));
+ when(spyTimelineReaderClient.getApplicationEntity(appId2, "ALL", null))
+ .thenReturn(createApplicationTimelineEntity(appId2, true,
+ false));
+ when(spyTimelineReaderClient.getApplicationEntity(appMultiple, "ALL",
null))
+ .thenReturn(createApplicationTimelineEntity(appMultiple, true,
+ false));
+
+ List<ContainerReport> containerList1 = client.getContainers(appAttemptId1);
+ List<ContainerReport> containerList2 = client.getContainers(appAttemptId2);
+
+ List<ContainerReport> containerListMultiple =
client.getContainers(appAttemptIdMultiple);
+
+ assertThat(containerList1.size()).isEqualTo(1);
+ assertThat(containerList2.size()).isEqualTo(1);
+ assertThat(containerListMultiple.size()).isEqualTo(multipleContainerSize);
+
+ assertThat(containerList1.get(0).getContainerId().getApplicationAttemptId()
+ .getApplicationId().getId()).isEqualTo(1);
+ assertThat(containerList2.get(0).getContainerId().getApplicationAttemptId()
+ .getApplicationId().getId()).isEqualTo(2);
Review comment:
Can you please use a variable for the applicationAttemptId in the
isEqualTo part? It's easier to read that way.
--
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]
