You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by govind-menon <gi...@git.apache.org> on 2018/12/26 17:20:14 UTC

[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

GitHub user govind-menon opened a pull request:

    https://github.com/apache/storm/pull/2930

    STORM-3274: Migrates storm CLI to using argparse making documentation…

    … more accessible

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/govind-menon/storm STORM-3274

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/storm/pull/2930.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 #2930
    
----
commit a5584814670beba3f2ce927f99be33f0d37edbe5
Author: Govind Menon <go...@...>
Date:   2018-12-26T17:18:33Z

    STORM-3274: Migrates storm CLI to using argparse making documentation more accessible

----


---

[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

Posted by srdo <gi...@git.apache.org>.
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 pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

Posted by srdo <gi...@git.apache.org>.
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...

Posted by govind-menon <gi...@git.apache.org>.
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 @@
                         </execution>
                     </executions>
                 </plugin>
    +            <plugin>
    +            <groupId>org.codehaus.mojo</groupId>
    +            <artifactId>exec-maven-plugin</artifactId>
    +            <executions>
    +                <execution>
    +                    <configuration>
    +                        <executable>python2.7</executable>
    --- 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


---

[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by govind-menon <gi...@git.apache.org>.
Github user govind-menon commented on the issue:

    https://github.com/apache/storm/pull/2930
  
    Tests will be forthcoming


---

[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:

    https://github.com/apache/storm/pull/2930
  
    Thanks. It looks great. A few final things:
    
    * When running `mvn clean install -DskipTests`, the tests for the script are still run. Can we skip the executions if `skipTests` is true?
    * Running the tests still generates a .pyc file in storm/bin, which ends up in the distribution. I think we should either remove it after the tests run, or explicitly exclude it from the storm-dist build.
    * The help is sorted alphabetically, which is good. Can we do the same for the positional arguments (e.g. jar, logviewer, kill)?
    * I would still like us to describe the new dependency on `mock` in DEVELOPER.md, as mentioned here https://github.com/apache/storm/pull/2930#issuecomment-452016830. I think it is ideal if people can blindly follow the setup guide and build Storm, having the build fail because there are extra setup steps that aren't mentioned in the guide could be demotivating to new people.


---

[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

Posted by srdo <gi...@git.apache.org>.
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 @@
                         </execution>
                     </executions>
                 </plugin>
    +            <plugin>
    +            <groupId>org.codehaus.mojo</groupId>
    +            <artifactId>exec-maven-plugin</artifactId>
    +            <executions>
    +                <execution>
    +                    <configuration>
    +                        <executable>python2.7</executable>
    --- End diff --
    
    Or even better, set the executable name to python2.7 on Linux, and python on Windows.


---

[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:

    https://github.com/apache/storm/pull/2930
  
    The .pyc issue can be solved by adding `<PYTHONDONTWRITEBYTECODE>true</PYTHONDONTWRITEBYTECODE>` to the environment variables for the two `exec-maven-plugin` executions


---

[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by srdo <gi...@git.apache.org>.
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...

Posted by srdo <gi...@git.apache.org>.
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 issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by govind-menon <gi...@git.apache.org>.
Github user govind-menon commented on the issue:

    https://github.com/apache/storm/pull/2930
  
    @srdo 
    
    Addressed most of your comments
    
    - Tests are run for python2 and python 3
    - Help is sorted alphabetically now
    - running just storm prints the entire help
    - The mutual exclusion change isn't doable if there are other positional arguments (which there are for that command)


---

[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:

    https://github.com/apache/storm/pull/2930
  
    @govind-menon The `node-health-check` command  is missing from `storm.py` after this change.


---

[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

Posted by srdo <gi...@git.apache.org>.
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 issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by govind-menon <gi...@git.apache.org>.
Github user govind-menon commented on the issue:

    https://github.com/apache/storm/pull/2930
  
    @srdo I've addressed the 4 comments you had - thank you for the review


---

[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:

    https://github.com/apache/storm/pull/2930#discussion_r245480567
  
    --- Diff: storm-client/pom.xml ---
    @@ -240,6 +240,29 @@
                         </execution>
                     </executions>
                 </plugin>
    +            <plugin>
    +            <groupId>org.codehaus.mojo</groupId>
    +            <artifactId>exec-maven-plugin</artifactId>
    +            <executions>
    +                <execution>
    +                    <configuration>
    +                        <executable>python2.7</executable>
    --- End diff --
    
    I'd like the storm-client build to still work on Windows. One option would be to only run this execution when the OS is UNIX-y, e.g.
    ```
    <profile>
        <activation>
            <os><family>unix</family></os>
        </activation>
        <plugin-config-here/>
    </profile>
    ```


---

[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

Posted by govind-menon <gi...@git.apache.org>.
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 @@
                         </execution>
                     </executions>
                 </plugin>
    +            <plugin>
    +            <groupId>org.codehaus.mojo</groupId>
    +            <artifactId>exec-maven-plugin</artifactId>
    +            <executions>
    +                <execution>
    +                    <configuration>
    +                        <executable>python2.7</executable>
    --- 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...

Posted by srdo <gi...@git.apache.org>.
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 @@
                         </execution>
                     </executions>
                 </plugin>
    +            <plugin>
    +            <groupId>org.codehaus.mojo</groupId>
    +            <artifactId>exec-maven-plugin</artifactId>
    +            <executions>
    +                <execution>
    +                    <configuration>
    +                        <executable>python2.7</executable>
    --- 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 issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by srdo <gi...@git.apache.org>.
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...

Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:

    https://github.com/apache/storm/pull/2930
  
    @govind-menon 
    The CLI is not recognizing -c options are configuration parameters.


---

[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:

    https://github.com/apache/storm/pull/2930
  
    Thanks, looks great. 
    
    The positional arguments don't seem to be sorted. I get
    
    ```
    usage: storm [-h] [--config CONFIG] [-storm_config_opts -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}
                 ...
    
    positional arguments:
      {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}
        jar                 Runs the main method of class with the specified
                            arguments. The storm worker dependencies 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/StormS
                            ubmitter.html) will upload the jar at topology-jar-
                            path when the topology is submitted. When you pass
                            jars and/or artifacts options, StormSubmitter will
                            upload them when the topology is submitted, and they
                            will be included to classpath of both the process
                            which runs the class, and also workers for that
                            topology.
        localconfvalue      Prints out the value for conf-name in the local Storm
                            configs. The local Storm configs are the ones in
                            ~/.storm/storm.yaml merged in with the configs in
                            defaults.yaml.
        remoteconfvalue     Prints out the value for conf-name in the cluster's
                            Storm configs. The cluster's Storm configs are the
                            ones in $STORM-PATH/conf/storm.yaml merged in with the
                            configs in defaults.yaml. This command must be run on
                            a cluster machine.
        local               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/apac
                            he/storm/StormSubmitter.html) and others will interact
                            with a local cluster instead of the one configured by
                            default. Most options should work just like with the
                            storm jar command.
    <snip>
    ```
    
    It's extremely minor though, and I'm not sure how to fix it. +1.


---

[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by srdo <gi...@git.apache.org>.
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 pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

Posted by srdo <gi...@git.apache.org>.
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)
    +    initialize_drpc_subcommand(subparsers)
    +    initialize_dev_zookeeper_subcommand(subparsers)
    +    initialize_version_subcommand(subparsers)
    +    initialize_classpath_subcommand(subparsers)
    +    initialize_server_classpath_subcommand(subparsers)
    +    initialize_monitor_subcommand(subparsers)
    +
    +    return main_parser
    +
    +
    +def initialize_localconfvalue_subcommand(subparsers):
    +    command_help = '''Prints out the value for conf-name in the local Storm configs.
    +    The local Storm configs are the ones in ~/.storm/storm.yaml merged
    +    in with the configs in defaults.yaml.'''
     
    -    Most options should work just like with the storm jar command.
    +    sub_parser = subparsers.add_parser("localconfvalue", help=command_help)
    +    sub_parser.add_argument("conf_name")
    +    sub_parser.set_defaults(func=print_localconfvalue)
     
    -    local also adds in the option --local-ttl which sets the number of seconds the
    -    local cluster will run for before it shuts down.
     
    -    --local-zookeeper if using an external zookeeper sets the connection string to use for it.
    +def initialize_remoteconfvalue_subcommand(subparsers):
    +    command_help = '''Prints out the value for conf-name in the cluster's Storm configs.
    +    The cluster's Storm configs are the ones in $STORM-PATH/conf/storm.yaml
    +    merged in with the configs in defaults.yaml.
     
    -    --java-debug lets you turn on java debugging and set the parameters passed to -agentlib:jdwp on the JDK
    -    --java-debug transport=dt_socket,address=localhost:8000
    -    will open up a debugging server on port 8000.
    -    """
    -    [ttl, lzk, debug_args, args] = parse_local_opts(args)
    -    extrajvmopts = ["-Dstorm.local.sleeptime=" + ttl]
    -    if lzk != None:
    -        extrajvmopts = extrajvmopts + ["-Dstorm.local.zookeeper=" + lzk]
    -    if debug_args != None:
    -        extrajvmopts = extrajvmopts + ["-agentlib:jdwp=" + debug_args]
    -    run_client_jar(jarfile, "org.apache.storm.LocalCluster", [klass] + list(args), client=False, daemon=False, extrajvmopts=extrajvmopts)
    -
    -def jar(jarfile, klass, *args):
    -    """Syntax: [storm jar topology-jar-path class ...]
    -
    -    Runs the main method of class with the specified arguments.
    -    The storm worker dependencies 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)
    -    will upload the jar at topology-jar-path when the topology is submitted.
    +    This command must be run on a cluster machine.'''
    +
    +    sub_parser = subparsers.add_parser("remoteconfvalue", help=command_help)
    +    sub_parser.add_argument("conf_name")
    +    sub_parser.set_defaults(func=print_remoteconfvalue)
    +
    +
    +def add_topology_jar_options(parser):
    +    parser.add_argument("topology_jar_path", help="will upload the jar at topology-jar-path when the topology is submitted.")
    +    parser.add_argument("topology_main_class", help="main class of the topology jar being submitted")
    +    parser.add_argument("topology_main_args", nargs='*', help="Runs the main method with the specified arguments.")
    +
    +
    +def add_client_jar_options(parser):
     
    -    When you want to ship other jars which is not included to application jar, you can pass them to --jars option with comma-separated string.
    +    parser.add_argument("--jars", help='''
    +    When you want to ship other jars which are not included to application jar, you can pass them to --jars option with comma-separated string.
         For example, --jars "your-local-jar.jar,your-local-jar2.jar" will load your-local-jar.jar and your-local-jar2.jar.
    -    And when you want to ship maven artifacts and its transitive dependencies, you can pass them to --artifacts with comma-separated string.
    +    ''', default="")
    +
    +    parser.add_argument("--artifacts", help='''
    +     When you want to ship maven artifacts and its transitive dependencies, you can pass them to --artifacts with comma-separated string.
         You can also exclude some dependencies like what you're doing in maven pom.
         Please add exclusion artifacts with '^' separated string after the artifact.
         For example, -artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka-clients:1.0.0^org.slf4j:slf4j-api" will load jedis and kafka-clients artifact and all of transitive dependencies but exclude slf4j-api from kafka.
    +        ''', default="")
    +
     
    -    When you need to pull the artifacts from other than Maven Central, you can pass remote repositories to --artifactRepositories option with comma-separated string.
    +    parser.add_argument("--artifactRepositories", help='''
    +    When you need to pull the artifacts from other than Maven Central, you can pass remote repositories to --artifactRepositories option with a comma-separated string.
         Repository format is "<name>^<url>". '^' is taken as separator because URL allows various characters.
         For example, --artifactRepositories "jboss-repository^http://repository.jboss.com/maven2,HDPRepo^http://repo.hortonworks.com/content/groups/public/" will add JBoss and HDP repositories for dependency resolver.
    -    You can provide local maven repository directory via --mavenLocalRepositoryDirectory if you would like to use specific directory. It might help when you don't have '.m2/repository' directory in home directory, because CWD is sometimes non-deterministic (fragile).
    +    ''', default="")
    +
    +    parser.add_argument("--mavenLocalRepositoryDirectory", help="You can provide local maven repository directory via --mavenLocalRepositoryDirectory if you would like to use specific directory. It might help when you don't have '.m2/repository' directory in home directory, because CWD is sometimes non-deterministic (fragile).", default="")
    +
     
    -    You can also provide proxy information to let dependency resolver utilizing proxy if needed. There're three parameters for proxy:
    -    --proxyUrl: URL representation of proxy ('http://host:port')
    -    --proxyUsername: username of proxy if it requires basic auth
    -    --proxyPassword: password of proxy if it requires basic auth
    +    parser.add_argument("--proxyUrl", help="You can also provide proxy information to let dependency resolver utilizing proxy if needed. URL representation of proxy ('http://host:port')", default="")
    +    parser.add_argument("--proxyUsername", help="username of proxy if it requires basic auth", default="")
    +    parser.add_argument("--proxyPassword", help="password of proxy if it requires basic auth", default="")
     
    -    Complete example of options is here: `./bin/storm jar example/storm-starter/storm-starter-topologies-*.jar org.apache.storm.starter.RollingTopWords blobstore-remote2 remote --jars "./external/storm-redis/storm-redis-1.1.0.jar,./external/storm-kafka-client/storm-kafka-client-1.1.0.jar" --artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka-clients:1.0.0^org.slf4j:slf4j-api" --artifactRepositories "jboss-repository^http://repository.jboss.com/maven2,HDPRepo^http://repo.hortonworks.com/content/groups/public/"`
    +
    +def initialize_jar_subcommand(subparsers):
    +    jar_help = """Runs the main method of class with the specified arguments.
    +    The storm worker dependencies 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)
    +    will upload the jar at topology-jar-path when the topology is submitted.
     
         When you pass jars and/or artifacts options, StormSubmitter will upload them when the topology is submitted, and they will be included to classpath of both the process which runs the class, and also workers for that topology.
    +    """
    +    jar_parser = subparsers.add_parser("jar", help=jar_help)
    +
    +    add_topology_jar_options(jar_parser)
    +    add_client_jar_options(jar_parser)
    +
    +    jar_parser.add_argument(
    +        "--storm-server-classpath",
    +        action='store_true',
    +        help='''
    +        If for some reason you need to have the full storm classpath,
    +        not just the one for the worker you may include the command line option `--storm-server-classpath`.
    +        Please be careful because this will add things to the classpath
    +        that will not be on the worker classpath
    +        and could result in the worker not running.'''
    +    )
    +
    +    jar_parser.set_defaults(func=jar)
    +
    +
    +def initialize_local_subcommand(subparsers):
    +    command_help = """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.
    +
    +    Most options should work just like with the storm jar command.
    +    """
    +    sub_parser = subparsers.add_parser("local", help=command_help)
    +
    +    add_topology_jar_options(sub_parser)
    +    add_client_jar_options(sub_parser)
    +
    +    sub_parser.add_argument(
    +        "--local-ttl",
    +        help="sets the number of seconds the local cluster will run for before it shuts down",
    +        default=LOCAL_TTL_DEFAULT
    +    )
    +
    +    sub_parser.add_argument(
    +        "--java-debug",
    +        help="lets you turn on java debugging and set the parameters passed to -agentlib:jdwp on the JDK" +
    +             "e.g transport=dt_socket,address=localhost:8000 will open up a debugging server on port 8000",
    +        default=None
    +    )
    +
    +    sub_parser.set_defaults(func=local)
    +
    +
    +def initialize_kill_subcommand(subparsers):
    +    command_help = """Kills the topology with the name topology-name. Storm will
    +    first deactivate the topology's spouts for the duration of
    +    the topology's message timeout to allow all messages currently
    +    being processed to finish processing. Storm will then shutdown
    +    the workers and clean up their state.
    +    """
    +    sub_parser = subparsers.add_parser("kill", help=command_help)
    +
    +    sub_parser.add_argument("topology-name")
    +
    +    sub_parser.add_argument(
    +        "-w", "--wait-time-secs",
    +        help="""override the length of time Storm waits between deactivation and shutdown""",
    +        default=None
    +    )
    +
    +    sub_parser.set_defaults(func=kill)
    +
    +
    +def check_positive(value):
    +    ivalue = int(value)
    +    if ivalue <= 0:
    +        raise argparse.ArgumentTypeError("%s is not a positive integer" % value)
    +    return ivalue
    +
    +def check_even_list(cred_list):
    +    if not (len(cred_list) % 2):
    +        raise argparse.ArgumentTypeError("please provide a list of cred key and value pairs")
    +    return cred_list
    +
    +
    +def initialize_upload_credentials_subcommand(subparsers):
    +    command_help = """Uploads a new set of credentials to a running topology."""
    +    sub_parser = subparsers.add_parser("upload-credentials", help=command_help)
    +
    +    sub_parser.add_argument("topology-name")
    +
    +    sub_parser.add_argument(
    +        "-f", "--file", default=None,
    +        help="""provide a properties file with credentials in it to be uploaded"""
    +    )
    +
    +    sub_parser.add_argument(
    +        "-u", "--user", default=None,
    +        help="""name of the owner of the topology (security precaution)"""
    +    )
    +
    +    sub_parser.add_argument(
    +        "cred_list", nargs='*', help="List of credkeys and their values [credkey credvalue]*",
    +        type=check_even_list
    +    )
    +
    +    sub_parser.set_defaults(func=upload_credentials)
    +
    +
    +def initialize_sql_subcommand(subparsers):
    +    command_help = """Compiles the SQL statements into a Trident topology and submits it to Storm.
    +    If user activates explain mode, SQL Runner analyzes each query statement
    +    and shows query plan instead of submitting topology.
    +    """
    +
    +    sub_parser = subparsers.add_parser("sql", help=command_help)
    +
    +    add_client_jar_options(sub_parser)
    +
    +    sub_parser.add_argument("sql_file", metavar="sql-file")
    +    sub_parser.add_argument(
    +        "topology_name", metavar="topology-name", help="should be --explain to activate explain mode"
    --- End diff --
    
    Nit: Can we use an argument group to do this instead of putting --explain into topology-name? https://docs.python.org/3/library/argparse.html#mutual-exclusion


---

[GitHub] storm pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

Posted by srdo <gi...@git.apache.org>.
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 pull request #2930: STORM-3274: Migrates storm CLI to using argparse m...

Posted by govind-menon <gi...@git.apache.org>.
Github user govind-menon commented on a diff in the pull request:

    https://github.com/apache/storm/pull/2930#discussion_r247197760
  
    --- 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)
    +    initialize_drpc_subcommand(subparsers)
    +    initialize_dev_zookeeper_subcommand(subparsers)
    +    initialize_version_subcommand(subparsers)
    +    initialize_classpath_subcommand(subparsers)
    +    initialize_server_classpath_subcommand(subparsers)
    +    initialize_monitor_subcommand(subparsers)
    +
    +    return main_parser
    +
    +
    +def initialize_localconfvalue_subcommand(subparsers):
    +    command_help = '''Prints out the value for conf-name in the local Storm configs.
    +    The local Storm configs are the ones in ~/.storm/storm.yaml merged
    +    in with the configs in defaults.yaml.'''
     
    -    Most options should work just like with the storm jar command.
    +    sub_parser = subparsers.add_parser("localconfvalue", help=command_help)
    +    sub_parser.add_argument("conf_name")
    +    sub_parser.set_defaults(func=print_localconfvalue)
     
    -    local also adds in the option --local-ttl which sets the number of seconds the
    -    local cluster will run for before it shuts down.
     
    -    --local-zookeeper if using an external zookeeper sets the connection string to use for it.
    +def initialize_remoteconfvalue_subcommand(subparsers):
    +    command_help = '''Prints out the value for conf-name in the cluster's Storm configs.
    +    The cluster's Storm configs are the ones in $STORM-PATH/conf/storm.yaml
    +    merged in with the configs in defaults.yaml.
     
    -    --java-debug lets you turn on java debugging and set the parameters passed to -agentlib:jdwp on the JDK
    -    --java-debug transport=dt_socket,address=localhost:8000
    -    will open up a debugging server on port 8000.
    -    """
    -    [ttl, lzk, debug_args, args] = parse_local_opts(args)
    -    extrajvmopts = ["-Dstorm.local.sleeptime=" + ttl]
    -    if lzk != None:
    -        extrajvmopts = extrajvmopts + ["-Dstorm.local.zookeeper=" + lzk]
    -    if debug_args != None:
    -        extrajvmopts = extrajvmopts + ["-agentlib:jdwp=" + debug_args]
    -    run_client_jar(jarfile, "org.apache.storm.LocalCluster", [klass] + list(args), client=False, daemon=False, extrajvmopts=extrajvmopts)
    -
    -def jar(jarfile, klass, *args):
    -    """Syntax: [storm jar topology-jar-path class ...]
    -
    -    Runs the main method of class with the specified arguments.
    -    The storm worker dependencies 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)
    -    will upload the jar at topology-jar-path when the topology is submitted.
    +    This command must be run on a cluster machine.'''
    +
    +    sub_parser = subparsers.add_parser("remoteconfvalue", help=command_help)
    +    sub_parser.add_argument("conf_name")
    +    sub_parser.set_defaults(func=print_remoteconfvalue)
    +
    +
    +def add_topology_jar_options(parser):
    +    parser.add_argument("topology_jar_path", help="will upload the jar at topology-jar-path when the topology is submitted.")
    +    parser.add_argument("topology_main_class", help="main class of the topology jar being submitted")
    +    parser.add_argument("topology_main_args", nargs='*', help="Runs the main method with the specified arguments.")
    +
    +
    +def add_client_jar_options(parser):
     
    -    When you want to ship other jars which is not included to application jar, you can pass them to --jars option with comma-separated string.
    +    parser.add_argument("--jars", help='''
    +    When you want to ship other jars which are not included to application jar, you can pass them to --jars option with comma-separated string.
         For example, --jars "your-local-jar.jar,your-local-jar2.jar" will load your-local-jar.jar and your-local-jar2.jar.
    -    And when you want to ship maven artifacts and its transitive dependencies, you can pass them to --artifacts with comma-separated string.
    +    ''', default="")
    +
    +    parser.add_argument("--artifacts", help='''
    +     When you want to ship maven artifacts and its transitive dependencies, you can pass them to --artifacts with comma-separated string.
         You can also exclude some dependencies like what you're doing in maven pom.
         Please add exclusion artifacts with '^' separated string after the artifact.
         For example, -artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka-clients:1.0.0^org.slf4j:slf4j-api" will load jedis and kafka-clients artifact and all of transitive dependencies but exclude slf4j-api from kafka.
    +        ''', default="")
    +
     
    -    When you need to pull the artifacts from other than Maven Central, you can pass remote repositories to --artifactRepositories option with comma-separated string.
    +    parser.add_argument("--artifactRepositories", help='''
    +    When you need to pull the artifacts from other than Maven Central, you can pass remote repositories to --artifactRepositories option with a comma-separated string.
         Repository format is "<name>^<url>". '^' is taken as separator because URL allows various characters.
         For example, --artifactRepositories "jboss-repository^http://repository.jboss.com/maven2,HDPRepo^http://repo.hortonworks.com/content/groups/public/" will add JBoss and HDP repositories for dependency resolver.
    -    You can provide local maven repository directory via --mavenLocalRepositoryDirectory if you would like to use specific directory. It might help when you don't have '.m2/repository' directory in home directory, because CWD is sometimes non-deterministic (fragile).
    +    ''', default="")
    +
    +    parser.add_argument("--mavenLocalRepositoryDirectory", help="You can provide local maven repository directory via --mavenLocalRepositoryDirectory if you would like to use specific directory. It might help when you don't have '.m2/repository' directory in home directory, because CWD is sometimes non-deterministic (fragile).", default="")
    +
     
    -    You can also provide proxy information to let dependency resolver utilizing proxy if needed. There're three parameters for proxy:
    -    --proxyUrl: URL representation of proxy ('http://host:port')
    -    --proxyUsername: username of proxy if it requires basic auth
    -    --proxyPassword: password of proxy if it requires basic auth
    +    parser.add_argument("--proxyUrl", help="You can also provide proxy information to let dependency resolver utilizing proxy if needed. URL representation of proxy ('http://host:port')", default="")
    +    parser.add_argument("--proxyUsername", help="username of proxy if it requires basic auth", default="")
    +    parser.add_argument("--proxyPassword", help="password of proxy if it requires basic auth", default="")
     
    -    Complete example of options is here: `./bin/storm jar example/storm-starter/storm-starter-topologies-*.jar org.apache.storm.starter.RollingTopWords blobstore-remote2 remote --jars "./external/storm-redis/storm-redis-1.1.0.jar,./external/storm-kafka-client/storm-kafka-client-1.1.0.jar" --artifacts "redis.clients:jedis:2.9.0,org.apache.kafka:kafka-clients:1.0.0^org.slf4j:slf4j-api" --artifactRepositories "jboss-repository^http://repository.jboss.com/maven2,HDPRepo^http://repo.hortonworks.com/content/groups/public/"`
    +
    +def initialize_jar_subcommand(subparsers):
    +    jar_help = """Runs the main method of class with the specified arguments.
    +    The storm worker dependencies 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)
    +    will upload the jar at topology-jar-path when the topology is submitted.
     
         When you pass jars and/or artifacts options, StormSubmitter will upload them when the topology is submitted, and they will be included to classpath of both the process which runs the class, and also workers for that topology.
    +    """
    +    jar_parser = subparsers.add_parser("jar", help=jar_help)
    +
    +    add_topology_jar_options(jar_parser)
    +    add_client_jar_options(jar_parser)
    +
    +    jar_parser.add_argument(
    +        "--storm-server-classpath",
    +        action='store_true',
    +        help='''
    +        If for some reason you need to have the full storm classpath,
    +        not just the one for the worker you may include the command line option `--storm-server-classpath`.
    +        Please be careful because this will add things to the classpath
    +        that will not be on the worker classpath
    +        and could result in the worker not running.'''
    +    )
    +
    +    jar_parser.set_defaults(func=jar)
    +
    +
    +def initialize_local_subcommand(subparsers):
    +    command_help = """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.
    +
    +    Most options should work just like with the storm jar command.
    +    """
    +    sub_parser = subparsers.add_parser("local", help=command_help)
    +
    +    add_topology_jar_options(sub_parser)
    +    add_client_jar_options(sub_parser)
    +
    +    sub_parser.add_argument(
    +        "--local-ttl",
    +        help="sets the number of seconds the local cluster will run for before it shuts down",
    +        default=LOCAL_TTL_DEFAULT
    +    )
    +
    +    sub_parser.add_argument(
    +        "--java-debug",
    +        help="lets you turn on java debugging and set the parameters passed to -agentlib:jdwp on the JDK" +
    +             "e.g transport=dt_socket,address=localhost:8000 will open up a debugging server on port 8000",
    +        default=None
    +    )
    +
    +    sub_parser.set_defaults(func=local)
    +
    +
    +def initialize_kill_subcommand(subparsers):
    +    command_help = """Kills the topology with the name topology-name. Storm will
    +    first deactivate the topology's spouts for the duration of
    +    the topology's message timeout to allow all messages currently
    +    being processed to finish processing. Storm will then shutdown
    +    the workers and clean up their state.
    +    """
    +    sub_parser = subparsers.add_parser("kill", help=command_help)
    +
    +    sub_parser.add_argument("topology-name")
    +
    +    sub_parser.add_argument(
    +        "-w", "--wait-time-secs",
    +        help="""override the length of time Storm waits between deactivation and shutdown""",
    +        default=None
    +    )
    +
    +    sub_parser.set_defaults(func=kill)
    +
    +
    +def check_positive(value):
    +    ivalue = int(value)
    +    if ivalue <= 0:
    +        raise argparse.ArgumentTypeError("%s is not a positive integer" % value)
    +    return ivalue
    +
    +def check_even_list(cred_list):
    +    if not (len(cred_list) % 2):
    +        raise argparse.ArgumentTypeError("please provide a list of cred key and value pairs")
    +    return cred_list
    +
    +
    +def initialize_upload_credentials_subcommand(subparsers):
    +    command_help = """Uploads a new set of credentials to a running topology."""
    +    sub_parser = subparsers.add_parser("upload-credentials", help=command_help)
    +
    +    sub_parser.add_argument("topology-name")
    +
    +    sub_parser.add_argument(
    +        "-f", "--file", default=None,
    +        help="""provide a properties file with credentials in it to be uploaded"""
    +    )
    +
    +    sub_parser.add_argument(
    +        "-u", "--user", default=None,
    +        help="""name of the owner of the topology (security precaution)"""
    +    )
    +
    +    sub_parser.add_argument(
    +        "cred_list", nargs='*', help="List of credkeys and their values [credkey credvalue]*",
    +        type=check_even_list
    +    )
    +
    +    sub_parser.set_defaults(func=upload_credentials)
    +
    +
    +def initialize_sql_subcommand(subparsers):
    +    command_help = """Compiles the SQL statements into a Trident topology and submits it to Storm.
    +    If user activates explain mode, SQL Runner analyzes each query statement
    +    and shows query plan instead of submitting topology.
    +    """
    +
    +    sub_parser = subparsers.add_parser("sql", help=command_help)
    +
    +    add_client_jar_options(sub_parser)
    +
    +    sub_parser.add_argument("sql_file", metavar="sql-file")
    +    sub_parser.add_argument(
    +        "topology_name", metavar="topology-name", help="should be --explain to activate explain mode"
    --- End diff --
    
    There are positional arguments added on L520 that don't allow mutual exclusion


---

[GitHub] storm issue #2930: STORM-3274: Migrates storm CLI to using argparse making d...

Posted by srdo <gi...@git.apache.org>.
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
    ```


---