[STORM-1273] port backtype.storm.cluster to java #1081
Conversation
hustfxj
commented
Feb 5, 2016
hustfxj
commented
- port cluster.clj to ClusterUtils/StormClusterState/StateStorage/ZKStateStorage/ZKStateStorageFactory
- Update all the callings to cluster
- Mock the java static functions for test purposes.
| } | ||
|
|
||
| public StormClusterState mkStormClusterStateImpl(Object StateStorage, List<ACL> acls, ClusterStateContext context) throws Exception { | ||
| if (StateStorage instanceof StateStorage) { |
There was a problem hiding this comment.
can you rename StateStorage variable to stateStorage (camel case)
Conflicts: storm-core/src/clj/org/apache/storm/cluster.clj storm-core/src/clj/org/apache/storm/cluster_state/zookeeper_state_factory.clj storm-core/src/clj/org/apache/storm/command/dev_zookeeper.clj storm-core/src/clj/org/apache/storm/daemon/common.clj storm-core/src/clj/org/apache/storm/daemon/executor.clj storm-core/src/clj/org/apache/storm/daemon/nimbus.clj storm-core/src/clj/org/apache/storm/daemon/supervisor.clj storm-core/src/clj/org/apache/storm/stats.clj storm-core/src/clj/org/apache/storm/testing.clj storm-core/src/clj/org/apache/storm/thrift.clj storm-core/src/clj/org/apache/storm/util.clj storm-core/src/clj/org/apache/storm/zookeeper.clj storm-core/test/clj/integration/org/apache/storm/integration_test.clj storm-core/test/clj/org/apache/storm/cluster_test.clj storm-core/test/clj/org/apache/storm/nimbus_test.clj storm-core/test/clj/org/apache/storm/supervisor_test.clj
|
@revans2 @abhishekagarwal87 can you help review these commits ? |
|
We would also like @knusbaum to review this one. |
| (finally | ||
| (.shutdown blob-store))) | ||
| (FileUtils/moveDirectory (File. tmproot) (File. stormroot)) | ||
| (try (FileUtils/moveDirectory (File. tmproot) (File. stormroot)) (catch Exception e)) |
There was a problem hiding this comment.
Why are we ignoring this exception now?
There was a problem hiding this comment.
Sorry, I forget remove the “try”. It can't pass testing at "FileUtils/moveDirectory" because the windows operating system. I will ignore this exception per test.
|
@hustfxj OK I will try to get through the entire thing today. So we can get it in soon. |
| public static List<ACL> mkTopoOnlyAcls(Map topoConf) throws NoSuchAlgorithmException { | ||
| List<ACL> aclList = null; | ||
| String payload = (String) topoConf.get(Config.STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD); | ||
| if (Utils.isZkAuthenticationConfiguredStormServer(topoConf)) { |
There was a problem hiding this comment.
This needs to be isZkAuthenticationConfiguredTopology not isZkAuthenticationConfiguredStormServer.
Original code is
(when (Utils/isZkAuthenticationConfiguredTopology topo-conf)
There was a problem hiding this comment.
@revans2 Thank you. I have addressed your comments.
| return executorWhb; | ||
| } | ||
|
|
||
| public IStormClusterState mkStormClusterStateImpl(Object stateStorage, List<ACL> acls, ClusterStateContext context) throws Exception { |
There was a problem hiding this comment.
Can we split this and mkStormClusterState into two methods? The signature of each are different enough that having them be separate is preferable.
There was a problem hiding this comment.
I agree with you. But it should be update into two methods after finishing some tests. Because it may happen some problems at tests when it has two methods.For example, (.mkStormClusterStateImpl (Mockito/verify cluster-utils (Mockito/times 1)) (Mockito/any) (Mockito/eq expected-acls) (Mockito/any))
There was a problem hiding this comment.
OK we can leave it for now. But please file a follow on JIRA so we can clean it up later.
|
OK I made it all of the way through. Things look fairly good, but there are a few places that need to be updated/fixed. |
|
@revans2 Thank you again. I have addressed your comments. |
|
+1 looks god to me. Did some manual testing and things look good. |
6 participants
