brumi1024 commented on a change in pull request #3763:
URL: https://github.com/apache/hadoop/pull/3763#discussion_r779638024



##########
File path: 
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAHSv2ClientImpl.java
##########
@@ -248,4 +290,36 @@ private static TimelineEntity 
createContainerEntity(ContainerId containerId) {
 
     return entity;
   }
+
+  private static List<TimelineEntity> createContainerEntities(ContainerId 
containerId) {
+    List<TimelineEntity> entities = new ArrayList<>();
+
+    TimelineEntity entity = new TimelineEntity();
+    entity.setType(ContainerMetricsConstants.ENTITY_TYPE);
+    entity.setId(containerId.toString());
+    Map<String, Object> entityInfo = new HashMap<String, Object>();
+    entityInfo.put(ContainerMetricsConstants.ALLOCATED_MEMORY_INFO, 1024);
+    entityInfo.put(ContainerMetricsConstants.ALLOCATED_VCORE_INFO, 8);
+    entityInfo.put(ContainerMetricsConstants.ALLOCATED_HOST_INFO,
+            "test host");
+    entityInfo.put(ContainerMetricsConstants.ALLOCATED_PORT_INFO, 100);
+    entityInfo
+            .put(ContainerMetricsConstants.ALLOCATED_PRIORITY_INFO, -1);
+    entityInfo.put(ContainerMetricsConstants
+            .ALLOCATED_HOST_HTTP_ADDRESS_INFO, "http://test:1234";);
+    entityInfo.put(ContainerMetricsConstants.DIAGNOSTICS_INFO,
+            "test diagnostics info");
+    entityInfo.put(ContainerMetricsConstants.EXIT_STATUS_INFO, -1);
+    entityInfo.put(ContainerMetricsConstants.STATE_INFO,
+            ContainerState.COMPLETE.toString());
+    entity.setInfo(entityInfo);
+
+    TimelineEvent tEvent = new TimelineEvent();
+    tEvent.setId(ContainerMetricsConstants.CREATED_IN_RM_EVENT_TYPE);
+    tEvent.setTimestamp(123456);
+    entity.addEvent(tEvent);
+
+    entities.add(entity);
+    return entities;
+  }

Review comment:
       This looks like the same as createContainerEntity except it wraps the 
entity in an array. I don't think the method should be duplicated, instead the 
original method could be called and then wrapped in an array.

##########
File path: 
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAHSv2ClientImpl.java
##########
@@ -107,6 +110,45 @@ public void testGetAppAttemptReport() throws IOException, 
YarnException {
         isEqualTo("test original tracking url");
   }
 
+  @Test
+  public void testGetContainerByAppAttempt() throws IOException, YarnException 
{
+    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);
+
+
+    Map<String, String> filter1 = new HashMap<>();
+    filter1.put("appattemptId", appAttemptId1.toString());
+    Map<String, String> filter2 = new HashMap<>();
+    filter2.put("appattemptId", appAttemptId2.toString());
+    when(spyTimelineReaderClient.getContainerEntities(appId1, "ALL", filter1,
+            0, null))
+            .thenReturn(createContainerEntities(containerId1));
+    when(spyTimelineReaderClient.getContainerEntities(appId2, "ALL", filter2,
+            0, null))
+            .thenReturn(createContainerEntities(containerId2));
+
+    when(spyTimelineReaderClient.getApplicationEntity(appId1, "ALL", null))
+            .thenReturn(createApplicationTimelineEntity(appId1, true,
+                    false));
+    when(spyTimelineReaderClient.getApplicationEntity(appId2, "ALL", null))
+            .thenReturn(createApplicationTimelineEntity(appId2, true,
+                    false));
+
+    List<ContainerReport> containerList1 = client.getContainers(appAttemptId1);
+    List<ContainerReport> containerList2 = client.getContainers(appAttemptId2);
+
+    assertThat(containerList1.size()).isEqualTo(1);
+    assertThat(containerList2.size()).isEqualTo(1);

Review comment:
       Can you please extend the checks to see whether the containerList 
contains the containers with the correct ids?

##########
File path: 
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAHSv2ClientImpl.java
##########
@@ -107,6 +110,45 @@ public void testGetAppAttemptReport() throws IOException, 
YarnException {
         isEqualTo("test original tracking url");
   }
 
+  @Test
+  public void testGetContainerByAppAttempt() throws IOException, YarnException 
{
+    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);
+
+
+    Map<String, String> filter1 = new HashMap<>();
+    filter1.put("appattemptId", appAttemptId1.toString());
+    Map<String, String> filter2 = new HashMap<>();
+    filter2.put("appattemptId", appAttemptId2.toString());

Review comment:
       Nit: Maybe an inline ImmutableMap.of would be a bit cleaner, because the 
filter maps aren't reused.

##########
File path: 
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAHSv2ClientImpl.java
##########
@@ -107,6 +110,45 @@ public void testGetAppAttemptReport() throws IOException, 
YarnException {
         isEqualTo("test original tracking url");
   }
 
+  @Test
+  public void testGetContainerByAppAttempt() throws IOException, YarnException 
{
+    final ApplicationId appId1 = ApplicationId.newInstance(0, 1);
+    final ApplicationId appId2 = ApplicationId.newInstance(0, 2);

Review comment:
       Can you please create an app with multiple attemps and multiple 
containers? To ensure only the requested appattempt's containers are returned.




-- 
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]

Reply via email to