APEXCORE-626 shutdown-app in cli supports app-name as argument #564

florianschmidt1994 · 2017-07-24T22:48:35Z

The command shutdown-app now supports not only the app-id but also the
app name as an argument.

This commit adds both the functionality as well as the documentation to
the cli help messages

pramodin · 2017-07-24T23:22:43Z

Are there any unit tests for shutdown?

florianschmidt1994 · 2017-07-24T23:27:30Z

I looked into ApexCliTest and ApexCliMiscTest and they both don't seem to contain tests related to the shutdown command

tushargosavi

Please address the provided comment. The reset looks good.

tushargosavi · 2017-07-25T03:53:26Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

+  {
+    ApplicationReport app = null;
+
+    if (getApplication(appNameOrId) != null) {


app = getApplication(appNameOrId)
if (app == null) {
app = getApplicationByName(appNameOrId)
}

To avoid calling getApplication and getApplicationByName twice.

tushargosavi · 2017-07-25T03:54:18Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

@@ -2178,6 +2178,19 @@ public void execute(String[] args, ConsoleReader reader) throws Exception

  }

+  private ApplicationReport findApplicationReportFromAppNameOrId(String appNameOrId)


this function could be used in KillAppCommand#execute too

tushargosavi · 2017-07-25T03:58:41Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

@@ -738,7 +738,7 @@ void printUsage(String cmd)
        "Kill a container"));
    connectedCommands.put("shutdown-app", new CommandSpec(new ShutdownAppCommand(),
        null,
-        new Arg[]{new VarArg("app-id")},
+        new Arg[]{new VarArg("app-id/app-name")},


Similar change is needed at
globalCommands.put("shutdown-app", new CommandSpec(new ShutdownAppCommand(),
new Arg[]{new Arg("app-id")},
new Arg[]{new VarArg("app-id")},

line number 634

tushargosavi · 2017-07-25T17:46:12Z

Looks good, can you squash all the commits and add JIRA id in the final commit message as per contributing guidelines (http://apex.apache.org/contributing.html). I will merge it after that

florianschmidt1994 · 2017-07-26T18:56:08Z

Jenkins, test me again!

sanjaypujare · 2017-07-27T10:58:58Z

I added the following comment to the JIRA and it will be nice to address it in this PR:

"Can we extend this to all the commands that accept the app-id? I see the following:

connect app-id
get-app-info app-id"

sanjaypujare · 2017-07-27T11:04:05Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

+    if (app == null) {
+      app = getApplicationByName(appNameOrId);
+    }
+    return app;


All the callers throw the identical exception when the returned app is null. Can we just throw that exception here and the caller can always assume app is non-null?

Thinking further, the current behavior - even with my proposed suggestion - fails the whole command if a single app-id/app-name is not found. Do we want to keep that behavior or modify it to only ignore the invalid app-id/app-name

The behavior seems to be inconsistent between different CL tools. I just checked docker start valid_container_name invalid_container_name: this would start valid_container but not invalid_container. Then again when using apt-get install valid_package_name invalid_package_name it just fails completely, installing neither of those packages.

Making findApplicationReportFromAppNameOrId() throw a CLIException could

on one hand clean up the code so we don't need to rewrite it every time

on the other hand it would make it inconsistent with getApplication and getApplicationByName, where no exception is thrown.

Also if we leave the exception out of the method, we would still have the possibility of implementing the logic for "fail all vs. fail only for invalid appName" in the respective commands, which we do not have if we move it into the method

IMO, it should be the best effort attempt. If application id or name is not found, the error needs to be reported, but it is necessary to continue to the next id/name. At the end, command should report all applications that were shutdown.

sanjaypujare · 2017-07-27T11:12:06Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

-            throw new CliException("Streaming application with id " + args[i] + " is not found.");
+          ApplicationReport ap = findApplicationReportFromAppNameOrId(args[i]);
+          if (ap == null) {
+            throw new CliException("Streaming application with id or name " + args[i] + " is not found.");


Pls see my comment above about failing the command for the whole list when a single app is not found.

sanjaypujare · 2017-07-27T11:13:05Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

-        new Arg[]{new Arg("app-id")},
-        new Arg[]{new VarArg("app-id")},
+        new Arg[]{new Arg("app-id/app-name")},
+        new Arg[]{new VarArg("app-id/app-name")},
        "Shutdown an app"));


=> "Shutdown app or apps"

sanjaypujare

I suggested extending this to other commands. Let me know what you think

florianschmidt1994 · 2017-07-27T17:16:05Z

Thanks for the reviews! ~~Adding the support for the other commands will make it more consistent, so I would go with adding that functionality everywhere.~~

EDIT: Actually it is tricky to add support for e.g get-app-info with using an app-name, since get-app-info also works for apps that are killed / shutdown / ... already and therefore multiple apps with that name can exist. Maybe open another issue / pr for those cases?

Regarding the question whether a command should fail completely or not if one of the apps is not found, I would go with not running the command at all, the reason being that it feels nicer to me in terms of usability, when one mistypes an app name or app id and wants to make a correction to it afterwards. (Instead of having to retype the whole command only for the failed app-names or app-ids). I'd be happy to get some more input on that though!

sanjaypujare · 2017-07-27T18:15:40Z

Okay, good point about multiple app-ids for the same name. Then it is not worth extending only for connect....

vrozov · 2017-07-27T18:27:26Z

@sanjaypujare @florianschmidt1994 Whether other commands can support application name in addition to application id should be considered separatly for each command. I'd prefer not to block this PR and have separate JIRAs and PRs for each command that can take application name as an argument in addition to application id.

tushargosavi · 2017-07-28T04:57:03Z

I agree with @vrozov to have separate Jiras for supporting app-name in other commands.

florianschmidt1994 · 2017-07-31T17:39:04Z

Are there any open issues left to do then?

tushargosavi

@sanjaypujare can you merge this if you don't have any additional comments.

vrozov · 2017-07-27T20:34:04Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

-        "Shutdown an app"));
+        new Arg[]{new Arg("app-id/app-name")},
+        new Arg[]{new VarArg("app-id/app-name")},
+        "Shutdown app or apps"));


Shutdown application(s) by id or name?

vrozov · 2017-08-01T17:19:51Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

+    if (app == null) {
+      app = getApplicationByName(appNameOrId);
+    }
+    return app;


IMO, it should be the best effort attempt. If application id or name is not found, the error needs to be reported, but it is necessary to continue to the next id/name. At the end, command should report all applications that were shutdown.

florianschmidt1994 · 2017-08-03T17:26:06Z

@vrozov I have both the best effort and the existing approach at hand. How / Who will it be decided with option we will go with? Do we need more input from others for that?

vrozov · 2017-08-04T12:49:48Z

@florianschmidt1994 The best approach is to bring the discussion to dev@apex. Once dev community comes to an agreement, summarize the discussion in JIRA.

florianschmidt1994 · 2017-08-07T21:58:07Z

Thanks for all your input! As discussed on the mailing list we will go with the "best effort" approach. The "latest" commit (thanks rebase :D ) contains the discussed functionality as well as an additional test to ensure proper behavior.
@sanjaypujare could you have a look at this again?

florianschmidt1994 · 2017-08-08T18:32:32Z

Run tests again

sanjaypujare · 2017-08-08T22:26:41Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

-          if (apps[i - 1] == null) {
-            throw new CliException("Streaming application with id " + args[i] + " is not found.");
-          }
+        String[] appNamesOrIds = Arrays.copyOfRange(args, 1, args.length);


Do we want to check for duplicates?

I'd vote for going with the way it currently behaves, which is shutting down an app if it is found, and if it is specified as argument twice: printing an error message on the second time, because the app is already shut down.

sanjaypujare · 2017-08-08T22:33:06Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

-            if (app == null) {
-              throw new CliException("Streaming application with id or name " + args[i] + " is not found.");
-            }
+            throw new CliException("Streaming application with id or name " + args[i] + " is not found.");


Why do we throw exception and stop processing the kill command as opposed to what we decided to do for the shutdown command e.g. continue processing the remaining arguments? In that case we can also achieve some code reuse between shutdown and kill commands - building the app report list etc can be common code.

sanjaypujare · 2017-08-08T22:33:57Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

          }
+
          yarnClient.killApplication(app.getApplicationId());


Same as above - if an exception is thrown the processing stops.

sanjaypujare · 2017-08-08T23:06:23Z

engine/src/test/java/com/datatorrent/stram/cli/ApexCliShutdownCommandTest.java

+    // When processing the shutdown command for two valid and one invalid appNames
+    String shutdownAppsCommand = "shutdown-app app1 notExisting app2";
+    cliUnderTest.processLine(shutdownAppsCommand, new ConsoleReader(), true);
+


Wouldn't it be a good idea to flush the current System.out and System.err (or close them) to ensure you get complete/consistent output in your stdOut and stdErr?

New squashed version uses autoflush on PrintStream now

sanjaypujare · 2017-08-08T23:07:55Z

engine/src/test/java/com/datatorrent/stram/cli/ApexCliShutdownCommandTest.java

+    "org.apache.log4j.*"
+    })
+@PrepareForTest({YarnClient.class, ApexCli.class, StramAgent.class})
+public class ApexCliShutdownCommandTest


How about adding a test for the kill command as well? You can add it here and rename the class .

Added a comment below on how to go from here

sanjaypujare

Pls check the comments

florianschmidt1994 · 2017-08-09T00:35:29Z

The current version now implements the best effort approach as discussed on the mailing list. Unfortunately this is now inconsistent to how the kill-app command behaves, being that kill-app stops when an error occurs during killing of an application.

I think there are two ways to go from here now:

Merge this PR (when approved), introducing inconsistent behavior for some time and fixing that inconsistent behavior for all other commands in a new JIRA / PR
Extend this PR & JIRA Issue to introduce consistent behavior for all commands that support multiple appNames / appIds as arguments

sanjaypujare · 2017-08-09T17:55:20Z

@florianschmidt1994 after thinking about this a bit, we should address the kill command inconsistency in a separate JIRA and merge this PR as soon as comments are addressed. Will finish my review....

sanjaypujare

LGTM

florianschmidt1994 · 2017-08-09T21:37:01Z

Added a minor change to fix a bug where the cli would wrongfully disconnect from currentApp

sanjaypujare · 2017-08-09T22:25:44Z

engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java

+      } finally {
+        if (currentApp != null) {
+          if (app.getApplicationId().equals(currentApp.getApplicationId())) {
+            currentApp = null;


In doing this you are assuming that shutdown succeeded or will succeed. If shutdown doesn't succeed why set currentApp to null?

pramod · 2017-08-09T23:58:18Z

Hey guys, I am @pramod on github but not the Pramod in your team. :-) Could you please remove me from this thread? Thanks, Pramod

…

On Wed, Aug 9, 2017 at 3:25 PM, sanjaypujare ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In engine/src/main/java/com/datatorrent/stram/cli/ApexCli.java <#564 (comment)>: > + System.err.printf(errMessage, appNameOrId, appNameOrId); + return; + } + + try { + JSONObject response = sendShutdownRequest(app); + if (consolePresent) { + System.out.printf("Shutdown of app %s requested: %s%n", app.getApplicationId().toString(), response); + } + } catch (Exception e) { + String errMessage = "Failed to request shutdown for app %s: %s%n"; + System.err.printf(errMessage, app.getApplicationId().toString(), e.getMessage()); + } finally { + if (currentApp != null) { + if (app.getApplicationId().equals(currentApp.getApplicationId())) { + currentApp = null; In doing this you are assuming that shutdown succeeded or will succeed. If shutdown doesn't succeed why set currentApp to null? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#564 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABbl0GthHoaVytXj-UWC8DMAqO9dMUZks5sWjHpgaJpZM4Oh1kS> .

florianschmidt1994 · 2017-08-10T00:30:28Z

My apologies, sorry about that! Unfortunately I think we can't do that, but you should be able to unsubscribe at the top right of the page, in the section notification. Also your email show have a link called "mute this thread", which should do the same.

vrozov · 2017-10-01T15:11:36Z

@florianschmidt1994 Please rebase

The command shutdown-app now supports not only the app-id but also the app name as an argument. In case of multiple appIds or appNames the shutdown-app command follows a "best effort" approach, meaning that it tries to shutdown each app individually and prints an error message if the shutdown of an app fails, but continues to shutdown all the others. This commit adds the described functionality, documentation to the cli help messages and a test to ensure correct best effort behaviour.

florianschmidt1994 · 2017-10-02T17:05:07Z

rebase @vrozov

florianschmidt1994 changed the title ~~APEXCORE-626.shutdown-app in cli supports app-name as argument~~ APEXCORE-626 shutdown-app in cli supports app-name as argument Jul 25, 2017

tushargosavi reviewed Jul 25, 2017

View reviewed changes

florianschmidt1994 force-pushed the APEXCORE-626.add-app-name-support-to-shutdown-app branch from 3175b88 to bc30b09 Compare July 25, 2017 17:56

florianschmidt1994 closed this Jul 26, 2017

florianschmidt1994 reopened this Jul 26, 2017

sanjaypujare reviewed Jul 27, 2017

View reviewed changes

sanjaypujare suggested changes Jul 27, 2017

View reviewed changes

florianschmidt1994 force-pushed the APEXCORE-626.add-app-name-support-to-shutdown-app branch from bc30b09 to f092ad9 Compare July 27, 2017 18:15

tushargosavi approved these changes Aug 1, 2017

View reviewed changes

vrozov reviewed Aug 1, 2017

View reviewed changes

florianschmidt1994 force-pushed the APEXCORE-626.add-app-name-support-to-shutdown-app branch 6 times, most recently from af20bee to f5677d3 Compare August 7, 2017 21:53

florianschmidt1994 force-pushed the APEXCORE-626.add-app-name-support-to-shutdown-app branch 5 times, most recently from b2473fd to 480d8b8 Compare August 8, 2017 18:09

florianschmidt1994 closed this Aug 8, 2017

florianschmidt1994 reopened this Aug 8, 2017

sanjaypujare reviewed Aug 8, 2017

View reviewed changes

sanjaypujare suggested changes Aug 8, 2017

View reviewed changes

florianschmidt1994 force-pushed the APEXCORE-626.add-app-name-support-to-shutdown-app branch from 480d8b8 to 8ccd1f0 Compare August 9, 2017 00:31

sanjaypujare approved these changes Aug 9, 2017

View reviewed changes

florianschmidt1994 force-pushed the APEXCORE-626.add-app-name-support-to-shutdown-app branch from 8ccd1f0 to 454e149 Compare August 9, 2017 21:35

sanjaypujare reviewed Aug 9, 2017

View reviewed changes

florianschmidt1994 force-pushed the APEXCORE-626.add-app-name-support-to-shutdown-app branch from 454e149 to 9ea2cfb Compare October 2, 2017 17:03

vrozov merged commit 10d22df into apache:master Oct 4, 2017

florianschmidt1994 deleted the APEXCORE-626.add-app-name-support-to-shutdown-app branch October 5, 2017 17:21

		@@ -2178,6 +2178,19 @@ public void execute(String[] args, ConsoleReader reader) throws Exception

		}

		private ApplicationReport findApplicationReportFromAppNameOrId(String appNameOrId)

APEXCORE-626 shutdown-app in cli supports app-name as argument #564

APEXCORE-626 shutdown-app in cli supports app-name as argument #564

Conversation

florianschmidt1994 commented Jul 24, 2017

pramodin commented Jul 24, 2017

florianschmidt1994 commented Jul 24, 2017

tushargosavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tushargosavi commented Jul 25, 2017

florianschmidt1994 commented Jul 26, 2017

sanjaypujare commented Jul 27, 2017

sanjaypujare Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjaypujare left a comment

Choose a reason for hiding this comment

florianschmidt1994 commented Jul 27, 2017 • edited Loading

sanjaypujare commented Jul 27, 2017

vrozov commented Jul 27, 2017

tushargosavi commented Jul 28, 2017

florianschmidt1994 commented Jul 31, 2017

tushargosavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florianschmidt1994 commented Aug 3, 2017

vrozov commented Aug 4, 2017

florianschmidt1994 commented Aug 7, 2017 • edited Loading

florianschmidt1994 commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjaypujare left a comment

Choose a reason for hiding this comment

florianschmidt1994 commented Aug 9, 2017 • edited Loading

sanjaypujare commented Aug 9, 2017

sanjaypujare left a comment

Choose a reason for hiding this comment

florianschmidt1994 commented Aug 9, 2017

Choose a reason for hiding this comment

pramod commented Aug 9, 2017 via email

florianschmidt1994 commented Aug 10, 2017 • edited Loading

vrozov commented Oct 1, 2017

florianschmidt1994 commented Oct 2, 2017

sanjaypujare Jul 27, 2017 •

edited

Loading

florianschmidt1994 commented Jul 27, 2017 •

edited

Loading

florianschmidt1994 commented Aug 7, 2017 •

edited

Loading

florianschmidt1994 commented Aug 9, 2017 •

edited

Loading

florianschmidt1994 commented Aug 10, 2017 •

edited

Loading