You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by GitBox <gi...@apache.org> on 2020/04/02 03:48:47 UTC

[GitHub] [storm] RuiLi8080 opened a new pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

RuiLi8080 opened a new pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402560113
 
 

 ##########
 File path: bin/storm.py
 ##########
 @@ -541,6 +541,13 @@ def initialize_upload_credentials_subcommand(subparsers):
         help="""name of the owner of the topology (security precaution)"""
     )
 
+    # If set, this flag will become true meaning that user expects non-empty creds to be uploaded.
+    # Command exits with non-zero code if uploaded creds collection is empty.
+    sub_parser.add_argument(
+        "-e", "--exception", action='store_true',
 
 Review comment:
   This is also good

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402566760
 
 

 ##########
 File path: storm-client/src/jvm/org/apache/storm/StormSubmitter.java
 ##########
 @@ -106,27 +106,30 @@ public static boolean validateZKDigestPayload(String payload) {
      * @param name        the name of the topology to push credentials to.
      * @param topoConf    the topology-specific configuration, if desired. See {@link Config}.
      * @param credentials the credentials to push.
+     * @return whether the pushed credential collection fullCreds is non-empty. Return false if empty.
 
 Review comment:
   `fullCreds` doesn't really mean anything and can be deleted; image anyone changed the variable name

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402329623
 
 

 ##########
 File path: storm-core/src/jvm/org/apache/storm/command/UploadCredentials.java
 ##########
 @@ -111,7 +112,13 @@ public static void main(String[] args) throws Exception {
         // use the local setting for the login config rather than the topology's
         topologyConf.remove("java.security.auth.login.config");
 
-        StormSubmitter.pushCredentials(topologyName, topologyConf, credentialsMap, (String) cl.get("u"));
+        boolean throwExceptionForEmptyCreds = (boolean) cl.getOrDefault("e", false);
 
 Review comment:
   No need for default value here. above `boolOpt` is already default to `false`

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402343950
 
 

 ##########
 File path: storm-client/src/jvm/org/apache/storm/StormSubmitter.java
 ##########
 @@ -126,7 +127,7 @@ public static void pushCredentials(String name, Map<String, Object> topoConf, Ma
      * @throws NotAliveException        if the topology is not alive
      * @throws InvalidTopologyException if any other error happens
      */
-    public static void pushCredentials(String name, Map<String, Object> topoConf, Map<String, String> credentials, String expectedUser)
+    public static boolean pushCredentials(String name, Map<String, Object> topoConf, Map<String, String> credentials, String expectedUser)
 
 Review comment:
   Fixed

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm merged pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm merged pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402343824
 
 

 ##########
 File path: bin/storm.py
 ##########
 @@ -541,6 +541,13 @@ def initialize_upload_credentials_subcommand(subparsers):
         help="""name of the owner of the topology (security precaution)"""
     )
 
+    # If set, this flag will become true meaning that user expects non-empty creds to be uploaded.
+    # Command exits with non-zero code if uploaded creds collection is empty.
+    sub_parser.add_argument(
+        "-e", "--exception", action='store_true',
 
 Review comment:
   I changed --exception-when-empty here since we already use short name `-f` for file. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402327646
 
 

 ##########
 File path: storm-client/src/jvm/org/apache/storm/StormSubmitter.java
 ##########
 @@ -126,7 +127,7 @@ public static void pushCredentials(String name, Map<String, Object> topoConf, Ma
      * @throws NotAliveException        if the topology is not alive
      * @throws InvalidTopologyException if any other error happens
      */
-    public static void pushCredentials(String name, Map<String, Object> topoConf, Map<String, String> credentials, String expectedUser)
+    public static boolean pushCredentials(String name, Map<String, Object> topoConf, Map<String, String> credentials, String expectedUser)
 
 Review comment:
   missing javadoc `@return`
   
   There is another `pushCredentials` method in this class needs to be taken care of

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r403274071
 
 

 ##########
 File path: bin/storm.py
 ##########
 @@ -541,6 +541,13 @@ def initialize_upload_credentials_subcommand(subparsers):
         help="""name of the owner of the topology (security precaution)"""
     )
 
+    # If set, this flag will become true meaning that user expects non-empty creds to be uploaded.
+    # Command exits with non-zero code if uploaded creds collection is empty.
+    sub_parser.add_argument(
+        "-e", "--exception", action='store_true',
 
 Review comment:
   @bipinprasad https://github.com/apache/storm/blob/088e722559437cbd55ae7073c7fb89ac0f8c4ea5/storm-core/src/jvm/org/apache/storm/command/UploadCredentials.java#L45

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r403064825
 
 

 ##########
 File path: bin/storm.py
 ##########
 @@ -541,6 +541,13 @@ def initialize_upload_credentials_subcommand(subparsers):
         help="""name of the owner of the topology (security precaution)"""
     )
 
+    # If set, this flag will become true meaning that user expects non-empty creds to be uploaded.
+    # Command exits with non-zero code if uploaded creds collection is empty.
+    sub_parser.add_argument(
+        "-e", "--exception-when-empty", action='store_true',
+        help="""whether to throw exception if there are no credentials uploaded, default to be false"""
 
 Review comment:
   Fixed
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402344364
 
 

 ##########
 File path: storm-core/src/jvm/org/apache/storm/command/UploadCredentials.java
 ##########
 @@ -111,7 +112,13 @@ public static void main(String[] args) throws Exception {
         // use the local setting for the login config rather than the topology's
         topologyConf.remove("java.security.auth.login.config");
 
-        StormSubmitter.pushCredentials(topologyName, topologyConf, credentialsMap, (String) cl.get("u"));
+        boolean throwExceptionForEmptyCreds = (boolean) cl.getOrDefault("e", false);
 
 Review comment:
   Fixed

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] bipinprasad commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r403305841
 
 

 ##########
 File path: bin/storm.py
 ##########
 @@ -541,6 +541,13 @@ def initialize_upload_credentials_subcommand(subparsers):
         help="""name of the owner of the topology (security precaution)"""
     )
 
+    # If set, this flag will become true meaning that user expects non-empty creds to be uploaded.
+    # Command exits with non-zero code if uploaded creds collection is empty.
+    sub_parser.add_argument(
+        "-e", "--exception", action='store_true',
 
 Review comment:
   Seems there is a slight problem. In python code, when exec_java_class() is called, the arguments to the java class is constructed partly from main_class_args. main_class_args is constructed by simply eliminating some arguments passed to the python code (and keeping the rest intact). So if python code got passed "-e" ii will get passed thru to java class as "-e". However, if "--exception was passed to python, then --exception will get passed to java. This wont match what java class is expecting. So in order to do a passthru, the option names should be the same, otherwise main_class_args should be suitably modified based on the args. Looks like passthru might be preferred solution.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402342853
 
 

 ##########
 File path: docs/Command-line-client.md
 ##########
 @@ -319,7 +319,9 @@ eg: `storm shell resources/ python topology.py arg1 arg2`
 
 Syntax: `storm upload_credentials topology-name [credkey credvalue]*`
 
-Uploads a new set of credentials to a running topology
+Uploads a new set of credentials to a running topology  
+   * `-e --exception`: optioanl flag. If set, command will fail and throw exception if no credentials were uploaded.
 
 Review comment:
   Fixed

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r404814761
 
 

 ##########
 File path: bin/storm.py
 ##########
 @@ -541,6 +541,13 @@ def initialize_upload_credentials_subcommand(subparsers):
         help="""name of the owner of the topology (security precaution)"""
     )
 
+    # If set, this flag will become true meaning that user expects non-empty creds to be uploaded.
+    # Command exits with non-zero code if uploaded creds collection is empty.
+    sub_parser.add_argument(
+        "-e", "--exception", action='store_true',
 
 Review comment:
   Yea, the argument in python is the same: "--exception-when-empty". 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402566760
 
 

 ##########
 File path: storm-client/src/jvm/org/apache/storm/StormSubmitter.java
 ##########
 @@ -106,27 +106,30 @@ public static boolean validateZKDigestPayload(String payload) {
      * @param name        the name of the topology to push credentials to.
      * @param topoConf    the topology-specific configuration, if desired. See {@link Config}.
      * @param credentials the credentials to push.
+     * @return whether the pushed credential collection fullCreds is non-empty. Return false if empty.
 
 Review comment:
   `fullCreds` doesn't really mean anything, image anyone changed the variable name

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402321623
 
 

 ##########
 File path: docs/Command-line-client.md
 ##########
 @@ -319,7 +319,9 @@ eg: `storm shell resources/ python topology.py arg1 arg2`
 
 Syntax: `storm upload_credentials topology-name [credkey credvalue]*`
 
-Uploads a new set of credentials to a running topology
+Uploads a new set of credentials to a running topology  
+   * `-e --exception`: optioanl flag. If set, command will fail and throw exception if no credentials were uploaded.
 
 Review comment:
   type "optioanl"

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402324888
 
 

 ##########
 File path: bin/storm.py
 ##########
 @@ -541,6 +541,13 @@ def initialize_upload_credentials_subcommand(subparsers):
         help="""name of the owner of the topology (security precaution)"""
     )
 
+    # If set, this flag will become true meaning that user expects non-empty creds to be uploaded.
+    # Command exits with non-zero code if uploaded creds collection is empty.
+    sub_parser.add_argument(
+        "-e", "--exception", action='store_true',
 
 Review comment:
   `--exception` is too general. Maybe `--fail-on-empty` or something

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r403064705
 
 

 ##########
 File path: docs/Command-line-client.md
 ##########
 @@ -319,7 +319,9 @@ eg: `storm shell resources/ python topology.py arg1 arg2`
 
 Syntax: `storm upload_credentials topology-name [credkey credvalue]*`
 
-Uploads a new set of credentials to a running topology
+Uploads a new set of credentials to a running topology  
+   * `-e --exception`: optional flag. If set, command will fail and throw exception if no credentials were uploaded.
 
 Review comment:
   Yes, fixed

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r403067060
 
 

 ##########
 File path: storm-client/src/jvm/org/apache/storm/StormSubmitter.java
 ##########
 @@ -106,27 +106,30 @@ public static boolean validateZKDigestPayload(String payload) {
      * @param name        the name of the topology to push credentials to.
      * @param topoConf    the topology-specific configuration, if desired. See {@link Config}.
      * @param credentials the credentials to push.
+     * @return whether the pushed credential collection fullCreds is non-empty. Return false if empty.
 
 Review comment:
   Sure. Fixed

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402565596
 
 

 ##########
 File path: bin/storm.py
 ##########
 @@ -541,6 +541,13 @@ def initialize_upload_credentials_subcommand(subparsers):
         help="""name of the owner of the topology (security precaution)"""
     )
 
+    # If set, this flag will become true meaning that user expects non-empty creds to be uploaded.
+    # Command exits with non-zero code if uploaded creds collection is empty.
+    sub_parser.add_argument(
+        "-e", "--exception-when-empty", action='store_true',
+        help="""whether to throw exception if there are no credentials uploaded, default to be false"""
 
 Review comment:
   It means true when this config is specified, otherwise it's false. 
   
   So more precisely, "If specified, throws an exception if no credentials are uploaded"

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r402561853
 
 

 ##########
 File path: docs/Command-line-client.md
 ##########
 @@ -319,7 +319,9 @@ eg: `storm shell resources/ python topology.py arg1 arg2`
 
 Syntax: `storm upload_credentials topology-name [credkey credvalue]*`
 
-Uploads a new set of credentials to a running topology
+Uploads a new set of credentials to a running topology  
+   * `-e --exception`: optional flag. If set, command will fail and throw exception if no credentials were uploaded.
 
 Review comment:
   This needs to be updated too

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r403067405
 
 

 ##########
 File path: storm-client/src/jvm/org/apache/storm/StormSubmitter.java
 ##########
 @@ -106,27 +106,30 @@ public static boolean validateZKDigestPayload(String payload) {
      * @param name        the name of the topology to push credentials to.
      * @param topoConf    the topology-specific configuration, if desired. See {@link Config}.
      * @param credentials the credentials to push.
+     * @return whether the pushed credential collection fullCreds is non-empty. Return false if empty.
 
 Review comment:
   only one Travis build failed. Should be irrelevant.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [storm] bipinprasad commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded

Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3243: [STORM-3616] add optional flag to fail upload-credentials cmd if no creds were uploaded
URL: https://github.com/apache/storm/pull/3243#discussion_r403018132
 
 

 ##########
 File path: bin/storm.py
 ##########
 @@ -541,6 +541,13 @@ def initialize_upload_credentials_subcommand(subparsers):
         help="""name of the owner of the topology (security precaution)"""
     )
 
+    # If set, this flag will become true meaning that user expects non-empty creds to be uploaded.
+    # Command exits with non-zero code if uploaded creds collection is empty.
+    sub_parser.add_argument(
+        "-e", "--exception", action='store_true',
 
 Review comment:
   Where is args.exception_when_empty passed to java command (in storm.py:upload_credentials()?
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services