You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by jacobtolar <gi...@git.apache.org> on 2018/10/15 20:33:10 UTC
[GitHub] storm pull request #2878: [STORM-3257] 'storm kill' command line should be a...
GitHub user jacobtolar opened a pull request:
https://github.com/apache/storm/pull/2878
[STORM-3257] 'storm kill' command line should be able to continue on error
Adds a new `-c` parameter to `storm kill`. When passed, we will attempt to kill all listed topologies even if an error occurs.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jacobtolar/storm STORM-3257
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2878.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2878
----
commit 961de217ba8912916f3d814997daf5b77e6608ad
Author: Jacob Tolar <jt...@...>
Date: 2018-10-15T20:29:13Z
[STORM-3257] 'storm kill' command line should be able to continue on error
----
---
[GitHub] storm pull request #2878: [STORM-3257] 'storm kill' command line should be a...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2878#discussion_r225358947
--- Diff: storm-core/src/jvm/org/apache/storm/command/KillTopology.java ---
@@ -25,21 +25,49 @@
public static void main(String[] args) throws Exception {
Map<String, Object> cl = CLI.opt("w", "wait", null, CLI.AS_INT)
+ .boolOpt("c", "continue-on-error")
--- End diff --
I suspect this conflicts with existing `-c` usage on https://github.com/apache/storm/blob/master/bin/storm.py#L1027 to specify general configuration parameters. I would use and `-i` `-ignore-errors` or something like that.
---
[GitHub] storm pull request #2878: [STORM-3257] 'storm kill' command line should be a...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/2878#discussion_r225584866
--- Diff: storm-core/src/jvm/org/apache/storm/command/KillTopology.java ---
@@ -25,21 +25,49 @@
public static void main(String[] args) throws Exception {
Map<String, Object> cl = CLI.opt("w", "wait", null, CLI.AS_INT)
+ .boolOpt("i", "ignore-errors")
.arg("TOPO", CLI.INTO_LIST)
.parse(args);
+
+ @SuppressWarnings("unchecked")
final List<String> names = (List<String>) cl.get("TOPO");
+
+ // wait seconds for topology to shut down
Integer wait = (Integer) cl.get("w");
+ // if '-i' set, we'll try to kill every topology listed, even if an error occurs
+ Boolean continueOnError = (Boolean) cl.get("i");
+
final KillOptions opts = new KillOptions();
if (wait != null) {
opts.set_wait_secs(wait);
}
+
NimbusClient.withConfiguredClient(new NimbusClient.WithNimbus() {
@Override
public void run(Nimbus.Iface nimbus) throws Exception {
+ int errorCount = 0;
for (String name : names) {
- nimbus.killTopologyWithOpts(name, opts);
- LOG.info("Killed topology: {}", name);
+ try {
+ nimbus.killTopologyWithOpts(name, opts);
+ LOG.info("Killed topology: {}", name);
+ } catch (Exception e) {
+ errorCount += 1;
+ if (!continueOnError) {
+ throw e;
+ } else {
+ LOG.info(
+ "Caught error killing topology '{}'; continuing as -i was passed. Exception: {}",
+ name,
+ e.getClass().getName()
+ );
+ }
+ }
+ }
+
+ // If we failed to kill any topology, still exit with failure status
+ if (errorCount > 0) {
+ throw new RuntimeException("Failed to successfully kill " + errorCount + " topologies.");
--- End diff --
Could we also list (log/output) the topology names for which the command failed? I am anticipating this is what the user would want to know whenever this happens.
---
[GitHub] storm pull request #2878: [STORM-3257] 'storm kill' command line should be a...
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on a diff in the pull request:
https://github.com/apache/storm/pull/2878#discussion_r228222037
--- Diff: storm-core/src/jvm/org/apache/storm/command/KillTopology.java ---
@@ -25,21 +25,49 @@
public static void main(String[] args) throws Exception {
Map<String, Object> cl = CLI.opt("w", "wait", null, CLI.AS_INT)
+ .boolOpt("i", "ignore-errors")
--- End diff --
do we even want/need this option? Any reason not to have this be the default behavior?
---
[GitHub] storm pull request #2878: [STORM-3257] 'storm kill' command line should be a...
Posted by jacobtolar <gi...@git.apache.org>.
Github user jacobtolar commented on a diff in the pull request:
https://github.com/apache/storm/pull/2878#discussion_r225377530
--- Diff: storm-core/src/jvm/org/apache/storm/command/KillTopology.java ---
@@ -25,21 +25,49 @@
public static void main(String[] args) throws Exception {
Map<String, Object> cl = CLI.opt("w", "wait", null, CLI.AS_INT)
+ .boolOpt("c", "continue-on-error")
--- End diff --
Ah, yes, that makes sense. 👍
---
[GitHub] storm pull request #2878: [STORM-3257] 'storm kill' command line should be a...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/2878#discussion_r225581522
--- Diff: storm-core/src/jvm/org/apache/storm/command/KillTopology.java ---
@@ -25,21 +25,49 @@
public static void main(String[] args) throws Exception {
Map<String, Object> cl = CLI.opt("w", "wait", null, CLI.AS_INT)
+ .boolOpt("i", "ignore-errors")
.arg("TOPO", CLI.INTO_LIST)
.parse(args);
+
+ @SuppressWarnings("unchecked")
final List<String> names = (List<String>) cl.get("TOPO");
+
+ // wait seconds for topology to shut down
--- End diff --
Maybe `Wait this many seconds after deactivating topology before killing`?
---
[GitHub] storm pull request #2878: [STORM-3257] 'storm kill' command line should be a...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2878#discussion_r225502712
--- Diff: storm-core/src/jvm/org/apache/storm/command/KillTopology.java ---
@@ -25,21 +25,49 @@
public static void main(String[] args) throws Exception {
Map<String, Object> cl = CLI.opt("w", "wait", null, CLI.AS_INT)
+ .boolOpt("i", "ignore-errors")
.arg("TOPO", CLI.INTO_LIST)
.parse(args);
+
+ @SuppressWarnings("unchecked")
final List<String> names = (List<String>) cl.get("TOPO");
+
+ // wait seconds for topology to shut down
Integer wait = (Integer) cl.get("w");
+ // if '-i' set, we'll try to kill every topology listed, even if an error occurs
+ Boolean continueOnError = (Boolean) cl.get("i");
+
final KillOptions opts = new KillOptions();
if (wait != null) {
opts.set_wait_secs(wait);
}
+
NimbusClient.withConfiguredClient(new NimbusClient.WithNimbus() {
@Override
public void run(Nimbus.Iface nimbus) throws Exception {
+ int errorCount = 0;
for (String name : names) {
- nimbus.killTopologyWithOpts(name, opts);
- LOG.info("Killed topology: {}", name);
+ try {
+ nimbus.killTopologyWithOpts(name, opts);
+ LOG.info("Killed topology: {}", name);
+ } catch (Exception e) {
+ errorCount += 1;
+ if (!continueOnError) {
+ throw e;
+ } else {
+ LOG.info(
+ "Caught error killing topology '{}'; continuing as -i was passed. Exception: {}",
--- End diff --
I would use LOG.error and let all error detais/stracktrace be printed instead of providing just Exception class name.
---
[GitHub] storm pull request #2878: [STORM-3257] 'storm kill' command line should be a...
Posted by danny0405 <gi...@git.apache.org>.
Github user danny0405 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2878#discussion_r225457257
--- Diff: storm-core/src/jvm/org/apache/storm/command/KillTopology.java ---
@@ -25,21 +25,49 @@
public static void main(String[] args) throws Exception {
Map<String, Object> cl = CLI.opt("w", "wait", null, CLI.AS_INT)
+ .boolOpt("i", "ignore-errors")
.arg("TOPO", CLI.INTO_LIST)
.parse(args);
+
+ @SuppressWarnings("unchecked")
final List<String> names = (List<String>) cl.get("TOPO");
+
+ // wait seconds for topology to shut down
Integer wait = (Integer) cl.get("w");
+ // if '-i' set, we'll try to kill every topology listed, even if an error occurs
+ Boolean continueOnError = (Boolean) cl.get("i");
--- End diff --
is set
---
[GitHub] storm pull request #2878: [STORM-3257] 'storm kill' command line should be a...
Posted by jacobtolar <gi...@git.apache.org>.
Github user jacobtolar commented on a diff in the pull request:
https://github.com/apache/storm/pull/2878#discussion_r228237268
--- Diff: storm-core/src/jvm/org/apache/storm/command/KillTopology.java ---
@@ -25,21 +25,49 @@
public static void main(String[] args) throws Exception {
Map<String, Object> cl = CLI.opt("w", "wait", null, CLI.AS_INT)
+ .boolOpt("i", "ignore-errors")
--- End diff --
I'm always a little wary of changing the default behavior of something. But I'd be happy to remove this option and just make this the default if that's acceptable.
---