[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-21 Thread revans2
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2208 @liu-zhaokun @Ethanlm is not a committer yet but I will try to pull it in. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your proj

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-20 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 @Ethanlm Hi,Ethan. @HeartSaVioR and @revans2 both approved this PR,could you help me merge it? --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-19 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2208 +1 for the change. @revans2 How about the last change? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not hav

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-19 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 @HeartSaVioR I have modify this PR follow @revans2 suggestion. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-19 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 @HeartSaVioR OK, I will do it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enab

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-19 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2208 @revans2 Thanks for the detailed explanation. I'm OK to leave this as it is, and maybe better to just add comment that it is the internal config and user shouldn't set it. @liu-zhaokun S

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-19 Thread revans2
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2208 Sorry I have been out of town or I would have joined the conversation sooner. Yes @HeartSaVioR is correct. The code is really confusing and overly complicated. I comes from when we were ad

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-18 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2208 OK now I'm seeing how it works... From topology side it's always set to "digest" from Storm Submitter, so it is not effectively configurable from topology level. From Daemon (Nim

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-18 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2208 @liu-zhaokun I didn't mean we should replace configuration with normal string. I meant if the value is constant, don't add it to Storm configuration (even config map) and use the constant value d

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-18 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 @harshach It has always been the case.In other words,the value of this configuration is digest all the time.You can see StormSubmitter.java,line 92.It doesn't matter even that is in case of

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-18 Thread harshach
Github user harshach commented on the issue: https://github.com/apache/storm/pull/2208 @HeartSaVioR wouldn't that be an issue incase of non-secure cluster if we are defaulting to "digest"? --- If your project is set up for it, you can reply to this email and have your reply appear on

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-17 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 @HeartSaVioR I have modified this PR according to your opinion,and it has passed all the test.Could you help me merge it?Thank you very much. --- If your project is set up for it, you can r

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-17 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 @HeartSaVioR Thanks for your reply.I will remove it from Config.java. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-17 Thread HeartSaVioR
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2208 I think the best way to avoid misleading is just removing the configuration and set the value as constant. We don't need to provide an option to user while the value is actually fixed. --- If y

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-17 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 @harshach Thanks for your reply.You can see StormSubmitter.java,line 91.STORM_ZOOKEEPER_TOPOLOGY_AUTH_SCHEME is should always be set to digest.It can't be and won't be other value.So,I think

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-17 Thread harshach
Github user harshach commented on the issue: https://github.com/apache/storm/pull/2208 @liu-zhaokun I think the comment there meant to say by default it will be "No Authentication". I.e Its users responsibility to set to digest in a secure clusters. But since the default settings for

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-17 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 @haitaoyao I am so sorry to bother you.Do you have time to help me review it? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well.

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-16 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 Can one of the admins verify this patch? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature

[GitHub] storm issue #2208: [STORM-2627] The annotation of storm.zookeeper.topology.a...

2017-07-14 Thread liu-zhaokun
Github user liu-zhaokun commented on the issue: https://github.com/apache/storm/pull/2208 @HeartSaVioR I am so sorry to bother you.Do you have time to help me review it? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well