You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by liu-zhaokun <gi...@git.apache.org> on 2017/06/26 11:55:17 UTC
[GitHub] storm pull request #2180: [STORM-2602] storm.zookeeper.topology.auth.payload...
GitHub user liu-zhaokun opened a pull request:
https://github.com/apache/storm/pull/2180
[STORM-2602] storm.zookeeper.topology.auth.payload doesn't work even you set it
[https://issues.apache.org/jira/browse/STORM-2602](https://issues.apache.org/jira/browse/STORM-2602)
"storm.zookeeper.topology.auth.payload" doesn't work even you have set it,because there doesn't use it when the value of STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD isn't null or other abnormal condition.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/liu-zhaokun/storm new5
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2180.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 #2180
----
commit 319c878f0f5e154ce2d8ecddcde0111e8c285642
Author: liuzhaokun <li...@zte.com.cn>
Date: 2017-06-26T11:53:03Z
[STORM-2602] storm.zookeeper.topology.auth.payload doesn't work even you set 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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2180: [STORM-2602] storm.zookeeper.topology.auth.payload...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2180#discussion_r124883536
--- Diff: storm-client/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -78,21 +78,18 @@ public static boolean validateZKDigestPayload(String payload) {
@SuppressWarnings("unchecked")
public static Map prepareZookeeperAuthentication(Map<String, Object> conf) {
Map toRet = new HashMap();
-
+ String secretPayload = (String) conf.get(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD);
// Is the topology ZooKeeper authentication configuration unset?
if (! conf.containsKey(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD) ||
conf.get(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD) == null ||
! validateZKDigestPayload((String)
conf.get(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD))) {
-
- String secretPayload = generateZookeeperDigestSecretPayload();
+ secretPayload = generateZookeeperDigestSecretPayload();
+ }
toRet.put(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD, secretPayload);
--- End diff --
Please adjust the indentation here.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
Could anyone help me merge this PR?It has been create long long ago.I only want to handle it properly.Could anyone reply to me?
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@HeartSaVioR
Did you miss this PR?Could you help me merge this PR?
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2180
+1
Btw, unfortunately we're busy with handling our own works as well, whether it is related to Storm project or not. I understand the pain when review process is dragging as I was a one of contributor for more than a year, but please take into account the fact that the review process for PR could be dragging even for months, which I really would like to avoid, but unfortunately we can't.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@hmcl
Could you help me merge this PR?
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@HeartSaVioR
Hello,I have update the test,and adjust my code according to @revans2 's opinion.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 on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@HeartSaVioR
Did you miss this PR?
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@revans2
Yes,I pass this configuration by storm.config.I will take a look at that test,and adjust my code according to your opinion.Thanks for your reply.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@HeartSaVioR
Thanks very much for your hard work.I know your difficulties now,and I will support your work.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2180
@liu-zhaokun
I read through the code and I would like to confirm how you are setting the payload. It looks like you are setting the payload in the storm.yaml file, and not passing it in on the command line, or setting it programmatically when submitting your topology. Is that correct?
If so then that is a use case I didn't anticipate when I first wrote the code and this change is fine, except for some indentation issues.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@HeartSaVioR
I want to use my payload by setting the configuration which named "STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD",but it doesn't work.The payload of any topology always be a uuid generated by the method which named generateZookeeperDigestSecretPayload().It isn't difficult to judge the logic of "prepareZookeeperAuthentication" was defective.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2180
On a related note this change looks to have broken
```
org.apache.storm.submitter-test / testname: test-md5-digest-secret-generation
```
So please take a look at why it broke. It probably just needs to be updated to always expect a payload to be returned.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@harshach
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 on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@srdo
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 on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@revans2
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 on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@HeartSaVioR
Could you help me merge this PR?
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@HeartSaVioR
Since this PR has been "+1"，could you help me merge IT now?
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@HeartSaVioR
Could you help me review this PR?I think it's a major bug.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/2180
+1
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2180
Sorry I don't know the detail of this part and not sure this is a bug. Have you faced specific issue regarding this ug?
cc. @revans2 I guess you might know about the detail, though the code block is 3 years old.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@HeartSaVioR
The new commit has passed all the tests.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2180: [STORM-2602] storm.zookeeper.topology.auth.payload...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2180
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2180: [STORM-2602] storm.zookeeper.topology.auth.payload...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2180#discussion_r124883491
--- Diff: storm-client/src/jvm/org/apache/storm/StormSubmitter.java ---
@@ -78,21 +78,18 @@ public static boolean validateZKDigestPayload(String payload) {
@SuppressWarnings("unchecked")
public static Map prepareZookeeperAuthentication(Map<String, Object> conf) {
Map toRet = new HashMap();
-
+ String secretPayload = (String) conf.get(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD);
// Is the topology ZooKeeper authentication configuration unset?
if (! conf.containsKey(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD) ||
conf.get(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD) == null ||
! validateZKDigestPayload((String)
conf.get(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD))) {
-
- String secretPayload = generateZookeeperDigestSecretPayload();
+ secretPayload = generateZookeeperDigestSecretPayload();
+ }
toRet.put(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD, secretPayload);
LOG.info("Generated ZooKeeper secret payload for MD5-digest: " + secretPayload);
--- End diff --
Please move this line up to be right below line 87. We don't want to log that we generated a payload unless we actually did.
---
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
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2180: [STORM-2602] storm.zookeeper.topology.auth.payload doesn'...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/storm/pull/2180
@vesense
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 on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---