[jira] [Commented] (YARN-2427) Add support for moving apps between queues in RM web services

["Zhijie Shen (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/YARN-2427?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14265310#comment-14265310 ] 

Zhijie Shen commented on YARN-2427:
-----------------------------------

Thanks for the patch, Varun! Some 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?
{code}
+  @GET
+  @Path("/apps/{appid}/queue")
+  @Produces({ MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML })
+  public AppQueue getAppQueue(@Context HttpServletRequest hsr,
+      @PathParam("appid") String appId) throws AuthorizationException {
{code}

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.
{code}
+    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());
+    }
{code}

3. If we avoided the logic in (2), we may have to handle ApplicationNotFoundException from ClientRMService#moveApplicationAcrossQueues and map it to NotFoundException around the following code.
{code}
+      if (ue.getCause() instanceof YarnException) {
+        YarnException ye = (YarnException) ue.getCause();
{code}

4. Make it "protected static"?
{code}
+  String appQueueToJSON(AppQueue targetQueue) throws Exception {
{code}

5. JAXBContextResolver needs to add AppQueue.

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

> 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
>
>
> 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
(v6.3.4#6332)