Junping Du commented on YARN-3240:

Thanks [~zjshen] for the patch! I am reviewing this patch now, and a couple of 
comments so far:
+  //TODO: It needs to be updated by the discovery service
   private URI resURI;
Looks like we are creating one TimelineClient for every application so we have 
multiple TimelineClients within NM. Do we think about the other way - one 
TimelineClient can talk to different app URLs (put url as a parameter in every 
call, so client can be more stateless)? I don't have any preference here and I 
think compatibility with old client could be a good reason here. But just 
curious on our decisions here.

In addition, I think we need a new constructor to take resURI as a parameter 
because this is not get from configuration now but get from caller of 
TimelineClient who know the resource details (address of aggregator). 
And a setter is also needed to resURI because when caller (AM or NMs) have any 
failure in PUT/POST (as IOException so far), its retry logic will notify RM to 
recovery (through heartbeat or allocate request, addressed in YARN-3039) and 
set it back afterwards.

catch (RuntimeException re) {
+      // runtime exception is expected if the client cannot connect the server
+      String msg =
+          "Failed to get the response from the timeline server.";
+      LOG.error(msg, re);
+      throw new IOException(re);
+    }
+    if (resp == null ||
+        resp.getClientResponseStatus() != ClientResponse.Status.OK) {
+      String msg =
+          "Failed to get the response from the timeline server.";
+      LOG.error(msg);
+      if (LOG.isDebugEnabled() && resp != null) {
+        String output = resp.getEntity(String.class);
+        LOG.debug("HTTP error code: " + resp.getStatus()
+            + " Server response : \n" + output);
+      }
+      throw new YarnException(msg);
+    }
Looks like we are differentiate 404 and 500 here with IOException and 
YarnException which looks fine to me. Do we plan to have different handling 
logic (in caller part) for two failure cases?

Other looks good to me.

> [Data Mode] Implement client API to put generic entities
> --------------------------------------------------------
>                 Key: YARN-3240
>                 URL: https://issues.apache.org/jira/browse/YARN-3240
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Zhijie Shen
>            Assignee: Zhijie Shen
>         Attachments: YARN-3240.1.patch, YARN-3240.2.patch

This message was sent by Atlassian JIRA

Reply via email to