Varun Vasudev updated YARN-2427:
    Attachment: apache-yarn-2427.4.patch

Thanks for the review Zhijie! I've uploaded a new patch - 
apache-yarn-2427.4.patch to address your comments.

1. Is GET operation necessary for queue info alone? We have already provided 
getting app API, which contains the queue information. It may be arguable that 
this response is smaller in terms of message size. However, full app meta-data 
should not be that bad, shouldn't it?

+  @GET
+  @Path("/apps/\{appid}/queue")
+  @Produces(\{ MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML })
+  public AppQueue getAppQueue(@Context HttpServletRequest hsr,
+      @PathParam("appid") String appId) throws AuthorizationException {

This was just for completeness. You are correct that the app info API gives the 
same information. I just figured that if we have a PUT for /queue, it would be 
nice to have a GET as well. If you feel strongly about it, I can remove it.

2. I assume the similar logic should be done in 
ClientRMService#moveApplicationAcrossQueues. If not, that method needs to be 
fixed. Here, we may not want to do the sanity check twice.

+    String userName = callerUGI.getUserName();
+    RMApp app = null;
+    try \{
+      app = getRMAppForAppId(appId);
+    } catch (NotFoundException e) \{
+      RMAuditLogger.logFailure(userName, AuditConstants.KILL_APP_REQUEST,
+        "UNKNOWN", "RMWebService", "Trying to move an absent application "
+            + appId);
+      throw e;
+    }
+    if (!app.getQueue().equals(targetQueue.getQueue())) \{
+      // user is attempting to change queue.
+      return moveApp(app, callerUGI, targetQueue.getQueue());
+    }


The reason for the existing app check is to have the appropriate audit log 
message. If the check is passed onto ClientRMService, it won't be possible to 
identify the source(web-service vs RPC) in the audit log.

4. Make it "protected static"?

+  String appQueueToJSON(AppQueue targetQueue) throws Exception {


bq. 5. JAXBContextResolver needs to add AppQueue.

Good catch. Fixed.

bq. 6. Is the code change in TestFifoScheduler and testAppSubmit necessary?

Just style improvements. I can remove them if you wish.

> Add support for moving apps between queues in RM web services
> -------------------------------------------------------------
>                 Key: YARN-2427
>                 URL: https://issues.apache.org/jira/browse/YARN-2427
>             Project: Hadoop YARN
>          Issue Type: New Feature
>          Components: resourcemanager
>            Reporter: Varun Vasudev
>            Assignee: Varun Vasudev
>         Attachments: apache-yarn-2427.0.patch, apache-yarn-2427.1.patch, 
> apache-yarn-2427.2.patch, apache-yarn-2427.3.patch, apache-yarn-2427.4.patch
> Support for moving apps from one queue to another is now present in 
> CapacityScheduler and FairScheduler. We should expose the functionality via 
> RM web services as well.

This message was sent by Atlassian JIRA

Reply via email to