[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2930#discussion_r245727413
--- Diff: bin/storm.py ---
@@ -296,787 +239,1044 @@ def exec_storm_class(klass, jvmtype="-server",
jvmopts=[], extrajars=[], args=[]
elif is_windows():
# handling whitespaces in JAVA_CMD
try:
-ret = sub.check_output(all_args, stderr=sub.STDOUT)
+ret = subprocess.check_output(all_args,
stderr=subprocess.STDOUT)
print(ret)
-except sub.CalledProcessError as e:
+except subprocess.CalledProcessError as e:
print(e.output)
sys.exit(e.returncode)
else:
os.execvp(JAVA_CMD, all_args)
return exit_code
-def run_client_jar(jarfile, klass, args, daemon=False, client=True,
extrajvmopts=[]):
-global DEP_JARS_OPTS, DEP_ARTIFACTS_OPTS,
DEP_ARTIFACTS_REPOSITORIES_OPTS, DEP_MAVEN_LOCAL_REPOSITORY_DIRECTORY,
DEP_PROXY_URL, DEP_PROXY_USERNAME, DEP_PROXY_PASSWORD
-local_jars = DEP_JARS_OPTS
-artifact_to_file_jars = resolve_dependencies(DEP_ARTIFACTS_OPTS,
DEP_ARTIFACTS_REPOSITORIES_OPTS, DEP_MAVEN_LOCAL_REPOSITORY_DIRECTORY,
DEP_PROXY_URL, DEP_PROXY_USERNAME, DEP_PROXY_PASSWORD)
+def run_client_jar(klass, args, daemon=False, client=True,
extrajvmopts=[]):
+local_jars = args.jars.split(",")
+jarfile = args.topology_jar_path
+
+artifact_to_file_jars = resolve_dependencies(
+args.artifacts, args.artifactRepositories,
+args.mavenLocalRepositoryDirectory, args.proxyUrl,
+args.proxyUsername, args.proxyPassword
+)
-extra_jars=[jarfile, USER_CONF_DIR, STORM_BIN_DIR]
+extra_jars = [jarfile, USER_CONF_DIR, STORM_BIN_DIR]
extra_jars.extend(local_jars)
extra_jars.extend(artifact_to_file_jars.values())
exec_storm_class(
-klass,
+klass, args.c,
--- End diff --
Can we rename `c` to something like `storm_config_opts`? If it's named that
way by argparse, then don't worry about it.
---
[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2930#discussion_r245744213
--- Diff: bin/storm.py ---
@@ -296,787 +239,1044 @@ def exec_storm_class(klass, jvmtype="-server",
jvmopts=[], extrajars=[], args=[]
elif is_windows():
# handling whitespaces in JAVA_CMD
try:
-ret = sub.check_output(all_args, stderr=sub.STDOUT)
+ret = subprocess.check_output(all_args,
stderr=subprocess.STDOUT)
print(ret)
-except sub.CalledProcessError as e:
+except subprocess.CalledProcessError as e:
print(e.output)
sys.exit(e.returncode)
else:
os.execvp(JAVA_CMD, all_args)
return exit_code
-def run_client_jar(jarfile, klass, args, daemon=False, client=True,
extrajvmopts=[]):
-global DEP_JARS_OPTS, DEP_ARTIFACTS_OPTS,
DEP_ARTIFACTS_REPOSITORIES_OPTS, DEP_MAVEN_LOCAL_REPOSITORY_DIRECTORY,
DEP_PROXY_URL, DEP_PROXY_USERNAME, DEP_PROXY_PASSWORD
-local_jars = DEP_JARS_OPTS
-artifact_to_file_jars = resolve_dependencies(DEP_ARTIFACTS_OPTS,
DEP_ARTIFACTS_REPOSITORIES_OPTS, DEP_MAVEN_LOCAL_REPOSITORY_DIRECTORY,
DEP_PROXY_URL, DEP_PROXY_USERNAME, DEP_PROXY_PASSWORD)
+def run_client_jar(klass, args, daemon=False, client=True,
extrajvmopts=[]):
+local_jars = args.jars.split(",")
+jarfile = args.topology_jar_path
+
+artifact_to_file_jars = resolve_dependencies(
+args.artifacts, args.artifactRepositories,
+args.mavenLocalRepositoryDirectory, args.proxyUrl,
+args.proxyUsername, args.proxyPassword
+)
-extra_jars=[jarfile, USER_CONF_DIR, STORM_BIN_DIR]
+extra_jars = [jarfile, USER_CONF_DIR, STORM_BIN_DIR]
extra_jars.extend(local_jars)
extra_jars.extend(artifact_to_file_jars.values())
exec_storm_class(
-klass,
+klass, args.c,
jvmtype="-client",
extrajars=extra_jars,
-args=args,
+args=args.topology_main_args,
daemon=False,
jvmopts=JAR_JVM_OPTS + extrajvmopts + ["-Dstorm.jar=" + jarfile] +
["-Dstorm.dependency.jars=" + ",".join(local_jars)] +
["-Dstorm.dependency.artifacts=" +
json.dumps(artifact_to_file_jars)])
-def local(jarfile, klass, *args):
-"""Syntax: [storm local topology-jar-path class ...]
-Runs the main method of class with the specified arguments but
pointing to a local cluster
-The storm jars and configs in ~/.storm are put on the classpath.
-The process is configured so that StormSubmitter
-
(http://storm.apache.org/releases/current/javadocs/org/apache/storm/StormSubmitter.html)
-and others will interact with a local cluster instead of the one
configured by default.
+def print_localconfvalue(args):
+print(args.conf_name + ": " + confvalue(args.conf_name, args.c,
[USER_CONF_DIR]))
+
+
+def print_remoteconfvalue(args):
+print(args.conf_name + ": " + confvalue(args.conf_name, args.c,
[CLUSTER_CONF_DIR]))
+
+
+def initialize_main_command():
+main_parser = argparse.ArgumentParser(prog="storm")
+main_parser.add_argument("--config", default=None, help="Override
default storm conf")
--- End diff --
Nit: It isn't obvious from the description what the difference between `-c`
and `--config` is, or how to use them. Can we update the descriptions? For
`--config` something like "Override default storm conf file", and for `-c`
something like "Override storm conf properties, e.g. key=val,key2=val2".
---
[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2930 Nit: The existing help message prints the commands alphabetically. We might want to do the same with the new help message. Quick google suggests it is possible https://stackoverflow.com/questions/12268602/sort-argparse-help-alphabetically. ---
[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2930
Can we make running `storm.py` with 0 arguments equivalent to running
`storm.py -h`? Running just `storm.py` gives the following output, which isn't
very nice:
```
PS E:\apache-storm-2.0.1-SNAPSHOT\bin> ./storm
usage: storm [-h] [--config CONFIG] [-c C]
{jar,localconfvalue,remoteconfvalue,local,sql,kill,upload-credentials,blobstore,heartbeats,activate,list,deactivate,rebalance,get-errors,kill_workers,admin,shell,repl,nimbus,pacemaker,supervisor,ui,logviewer,drpc-client,drpc,dev-zookeeper,version,classpath,server_classpath,monitor}
...
storm: error: too few arguments
```
---
[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2930 When I run the build, a storm.pyc file is generated in the storm/bin directory. This file ends up in the storm-dist distribution. I'm guessing this isn't intentional? ---
[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2930 Would it make sense to run the tests for Python 3 as well as Python 2.7? ---
[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2930 This is in reply to https://github.com/apache/storm/pull/2930#discussion_r245726485, for some reason Github won't let me put another comment there. Makes sense. I think we should at least note that `mock` is a prerequisite in https://github.com/apache/storm/blob/master/DEVELOPER.md#prerequisites. Maybe add a line that users should install `pip` and then do `pip install mock --user`. ---
[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...
Github user govind-menon commented on a diff in the pull request: https://github.com/apache/storm/pull/2930#discussion_r245726485 --- Diff: storm-client/pom.xml --- @@ -240,6 +240,29 @@ + +org.codehaus.mojo +exec-maven-plugin + + + +python2.7 --- End diff -- @srdo It's a little messier to do that and better to leave it up to the environment to do it's own test setup. --user is for then there's no virtualenv (which will vary from machine to machine) - which is a good example of how the host itself should do the setup. ---
[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2930#discussion_r245725371 --- Diff: storm-client/pom.xml --- @@ -240,6 +240,29 @@ + +org.codehaus.mojo +exec-maven-plugin + + + +python2.7 --- End diff -- Thanks, it looks good. I'm wondering if we can get away with also doing `pip install mock --user` as part of the Maven run, or is that a bad idea? ---
[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2930#discussion_r245722204
--- Diff: bin/storm.py ---
@@ -296,787 +239,1044 @@ def exec_storm_class(klass, jvmtype="-server",
jvmopts=[], extrajars=[], args=[]
elif is_windows():
# handling whitespaces in JAVA_CMD
try:
-ret = sub.check_output(all_args, stderr=sub.STDOUT)
+ret = subprocess.check_output(all_args,
stderr=subprocess.STDOUT)
print(ret)
-except sub.CalledProcessError as e:
+except subprocess.CalledProcessError as e:
print(e.output)
sys.exit(e.returncode)
else:
os.execvp(JAVA_CMD, all_args)
return exit_code
-def run_client_jar(jarfile, klass, args, daemon=False, client=True,
extrajvmopts=[]):
-global DEP_JARS_OPTS, DEP_ARTIFACTS_OPTS,
DEP_ARTIFACTS_REPOSITORIES_OPTS, DEP_MAVEN_LOCAL_REPOSITORY_DIRECTORY,
DEP_PROXY_URL, DEP_PROXY_USERNAME, DEP_PROXY_PASSWORD
-local_jars = DEP_JARS_OPTS
-artifact_to_file_jars = resolve_dependencies(DEP_ARTIFACTS_OPTS,
DEP_ARTIFACTS_REPOSITORIES_OPTS, DEP_MAVEN_LOCAL_REPOSITORY_DIRECTORY,
DEP_PROXY_URL, DEP_PROXY_USERNAME, DEP_PROXY_PASSWORD)
+def run_client_jar(klass, args, daemon=False, client=True,
extrajvmopts=[]):
+local_jars = args.jars.split(",")
+jarfile = args.topology_jar_path
+
+artifact_to_file_jars = resolve_dependencies(
+args.artifacts, args.artifactRepositories,
+args.mavenLocalRepositoryDirectory, args.proxyUrl,
+args.proxyUsername, args.proxyPassword
+)
-extra_jars=[jarfile, USER_CONF_DIR, STORM_BIN_DIR]
+extra_jars = [jarfile, USER_CONF_DIR, STORM_BIN_DIR]
extra_jars.extend(local_jars)
extra_jars.extend(artifact_to_file_jars.values())
exec_storm_class(
-klass,
+klass, args.c,
jvmtype="-client",
extrajars=extra_jars,
-args=args,
+args=args.topology_main_args,
daemon=False,
jvmopts=JAR_JVM_OPTS + extrajvmopts + ["-Dstorm.jar=" + jarfile] +
["-Dstorm.dependency.jars=" + ",".join(local_jars)] +
["-Dstorm.dependency.artifacts=" +
json.dumps(artifact_to_file_jars)])
-def local(jarfile, klass, *args):
-"""Syntax: [storm local topology-jar-path class ...]
-Runs the main method of class with the specified arguments but
pointing to a local cluster
-The storm jars and configs in ~/.storm are put on the classpath.
-The process is configured so that StormSubmitter
-
(http://storm.apache.org/releases/current/javadocs/org/apache/storm/StormSubmitter.html)
-and others will interact with a local cluster instead of the one
configured by default.
+def print_localconfvalue(args):
+print(args.conf_name + ": " + confvalue(args.conf_name, args.c,
[USER_CONF_DIR]))
+
+
+def print_remoteconfvalue(args):
+print(args.conf_name + ": " + confvalue(args.conf_name, args.c,
[CLUSTER_CONF_DIR]))
+
+
+def initialize_main_command():
+main_parser = argparse.ArgumentParser(prog="storm")
+main_parser.add_argument("--config", default=None, help="Override
default storm conf")
+main_parser.add_argument("-c", action="append", default=[],
help="Override storm conf properties")
+
+subparsers = main_parser.add_subparsers(help="")
+
+initialize_jar_subcommand(subparsers)
+initialize_localconfvalue_subcommand(subparsers)
+initialize_remoteconfvalue_subcommand(subparsers)
+initialize_local_subcommand(subparsers)
+initialize_sql_subcommand(subparsers)
+initialize_kill_subcommand(subparsers)
+initialize_upload_credentials_subcommand(subparsers)
+initialize_blobstore_subcommand(subparsers)
+initialize_heartbeats_subcommand(subparsers)
+initialize_activate_subcommand(subparsers)
+initialize_listtopos_subcommand(subparsers)
+initialize_deactivate_subcommand(subparsers)
+initialize_rebalance_subcommand(subparsers)
+initialize_get_errors_subcommand(subparsers)
+initialize_healthcheck_subcommand(subparsers)
+initialize_kill_workers_subcommand(subparsers)
+initialize_admin_subcommand(subparsers)
+initialize_shell_subcommand(subparsers)
+initialize_repl_subcommand(subparsers)
+initialize_nimbus_subcommand(subparsers)
+initialize_pacemaker_subcommand(subparsers)
+initialize_supervisor_subcommand(subparsers)
+initialize_ui_subcommand(subparsers)
+initialize_logviewer_subcommand(subparsers)
+initialize_drpc_client_subcommand(subparsers)
+
[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2930#discussion_r245714702
--- Diff: bin/storm.py ---
@@ -132,13 +56,8 @@ def get_jars_full(adir):
elif os.path.exists(adir):
files = [adir]
-ret = []
-for f in files:
-if f.endswith(".jar"):
-ret.append(os.path.join(adir, f))
-return ret
+return [os.path.join(adir, f) for f in files if f.endswith(".jar")]
-# If given path is a dir, make it a wildcard so the JVM will include all
JARs in the directory.
--- End diff --
Nit: Is this comment no longer relevant?
---
[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2930#discussion_r245715647 --- Diff: bin/storm.py --- @@ -156,49 +95,92 @@ def get_classpath(extrajars, daemon=True, client=False): ret.extend(get_wildcard_dir(os.path.join(STORM_DIR, "extlib"))) if daemon: ret.extend(get_wildcard_dir(os.path.join(STORM_DIR, "extlib-daemon"))) -if STORM_EXT_CLASSPATH != None: +if STORM_EXT_CLASSPATH: ret.append(STORM_EXT_CLASSPATH) -if daemon and STORM_EXT_CLASSPATH_DAEMON != None: +if daemon and STORM_EXT_CLASSPATH_DAEMON: ret.append(STORM_EXT_CLASSPATH_DAEMON) ret.extend(extrajars) -return normclasspath(os.pathsep.join(ret)) +return NORMAL_CLASS_PATH(os.pathsep.join(ret)) -def confvalue(name, extrapaths, daemon=True): -global CONFFILE -command = [ -JAVA_CMD, "-client", get_config_opts(), "-Dstorm.conf.file=" + CONFFILE, -"-cp", get_classpath(extrapaths, daemon), "org.apache.storm.command.ConfigValue", name -] -p = sub.Popen(command, stdout=sub.PIPE) -output, errors = p.communicate() -# python 3 -if not isinstance(output, str): --- End diff -- Is this check not necessary for Python 3 anymore? ---
[GitHub] storm issue #2934: STORM-3310: Make JCQueueTest wait for consumer to read al...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2934 I'd recommend looking at the diff via https://github.com/apache/storm/pull/2934/files?w=1, since the indentation changes make it hard to tell what changed. ---
[GitHub] storm pull request #2934: STORM-3310: Make JCQueueTest wait for consumer to ...
GitHub user srdo opened a pull request: https://github.com/apache/storm/pull/2934 STORM-3310: Make JCQueueTest wait for consumer to read all queued ite⦠â¦ms before terminating https://issues.apache.org/jira/browse/STORM-3310 I've only seen the test fail once, but it should be pretty easy to see that the test has a race. The tests all expect some messages to go through the queue, but the `run` method doesn't make sure the producers have actually produced anything, and even if they do, the consumer may be shut down before it can read the messages the producers put in the queue. You can merge this pull request into a Git repository by running: $ git pull https://github.com/srdo/storm STORM-3310 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/2934.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 #2934 commit a84c879d3429afef946ab8e9c0d0cddf497e1c6d Author: Stig Rohde Døssing Date: 2019-01-07T16:11:23Z STORM-3310: Make JCQueueTest wait for consumer to read all queued items before terminating ---
[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...
Github user govind-menon commented on a diff in the pull request: https://github.com/apache/storm/pull/2930#discussion_r245709584 --- Diff: storm-client/pom.xml --- @@ -240,6 +240,29 @@ + +org.codehaus.mojo +exec-maven-plugin + + + +python2.7 --- End diff -- @srdo I've added a profile that would execute only on Unix - I didn't add one for Windows because that would change the file path separators that we're asserting on. I can change the test to make that configureable if you want ---
Re: Maintenance releases
Sounds good, thanks. Den man. 7. jan. 2019 kl. 15.58 skrev P. Taylor Goetz : > Not that I’m aware of. I’ll cue up the 1.x releases behind the 2.0 release. > > -Taylor > > > On Jan 7, 2019, at 3:58 AM, Stig Rohde Døssing > wrote: > > > > I think we should try to get the next 1.x releases out soon. Are there > any > > blockers? > >
Re: Maintenance releases
Not that I’m aware of. I’ll cue up the 1.x releases behind the 2.0 release. -Taylor > On Jan 7, 2019, at 3:58 AM, Stig Rohde Døssing wrote: > > I think we should try to get the next 1.x releases out soon. Are there any > blockers?
[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...
Github user srdo commented on a diff in the pull request: https://github.com/apache/storm/pull/2930#discussion_r245602260 --- Diff: storm-client/pom.xml --- @@ -240,6 +240,29 @@ + +org.codehaus.mojo +exec-maven-plugin + + + +python2.7 --- End diff -- Or even better, set the executable name to python2.7 on Linux, and python on Windows. ---
Maintenance releases
I think we should try to get the next 1.x releases out soon. Are there any blockers?
