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/06/24 15:24:56 UTC
[GitHub] [storm] RuiLi8080 opened a new pull request #3293: [STORM-3657] set storm.messaging.netty.authentication to topoConf OR daemonConf
RuiLi8080 opened a new pull request #3293:
URL: https://github.com/apache/storm/pull/3293
## What is the purpose of the change
In most of times of conf merging, we simply prefer topo conf over daemon conf. But as for netty authentication, we believe we want to set it true either cluster or topo owner want it.
We did notice some strange cases where we set this setting true in cluster side while user set it false when submitting topology. And when host-ip mapping changed, netty server of the worker with false setting receive message from other topology with true setting. The ControlMessage received at StormServerHandler can not be processed properly since false setting only expects `List<TaskMessage>`.
In the future, we should probably handle config better in storm since it would be weird for some config to be different between server and topo.
## How was the change tested
Tested with setting server side to true and submit a WordCount topo with false `-c` setting.
`storm jar storm-starter-2.3.0-SNAPSHOT.jar org.apache.storm.starter.WordCountTopology -c topology.debug=true -c storm.messaging.netty.authentication=false wc2`
Additional log:
```
2020-06-24 00:57:25.737 o.a.s.d.n.Nimbus pool-29-thread-47 [INFO] Topo conf: false, nimbus conf: true, Enforce netty auth: true
```
----------------------------------------------------------------
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
[GitHub] [storm] Ethanlm commented on pull request #3293: [STORM-3657] set storm.messaging.netty.authentication to topoConf OR daemonConf
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on pull request #3293:
URL: https://github.com/apache/storm/pull/3293#issuecomment-651172853
Travis failed on s390x. Not related
----------------------------------------------------------------
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
[GitHub] [storm] Ethanlm commented on a change in pull request #3293: [STORM-3657] set storm.messaging.netty.authentication to topoConf OR daemonConf
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3293:
URL: https://github.com/apache/storm/pull/3293#discussion_r445732035
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -3165,6 +3163,16 @@ public void submitTopologyWithOpts(String topoName, String uploadedJarLocation,
topoConf.remove(Config.TOPOLOGY_CLASSPATH_BEGINNING);
}
+ // storm.messaging.netty.authentication is about inter-worker communication
+ // enforce netty authentication when either topo or daemon set it to true
+ boolean enforceNettyAuth = (Boolean) topoConf.getOrDefault(Config.STORM_MESSAGING_NETTY_AUTHENTICATION, false)
Review comment:
`storm.messaging.netty.authentication` is guaranteed to be set because it is in `defaults.yaml`. We don't want to have default value in different places.
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -3165,6 +3163,16 @@ public void submitTopologyWithOpts(String topoName, String uploadedJarLocation,
topoConf.remove(Config.TOPOLOGY_CLASSPATH_BEGINNING);
}
+ // storm.messaging.netty.authentication is about inter-worker communication
Review comment:
I feel https://github.com/apache/storm/blob/master/storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java#L1098 is a better place for this.
----------------------------------------------------------------
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
[GitHub] [storm] Ethanlm merged pull request #3293: [STORM-3657] set storm.messaging.netty.authentication to topoConf OR daemonConf
Posted by GitBox <gi...@apache.org>.
Ethanlm merged pull request #3293:
URL: https://github.com/apache/storm/pull/3293
----------------------------------------------------------------
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