[STORM-1257] port backtype.storm.zookeeper to java #1047
Conversation
hustfxj
commented
Jan 26, 2016
hustfxj
commented
- Port "org.apache.storm.zookeeper" to java at "org.apache.storm.zookeeper.Zookeeper".
- Update all the callings to the config functions.
- Deleted most functions in zookeeper.clj except a few: mk-client (because of "watcher" which is derived by clojure in some .clj files).
- add the package "Callback", where callback functions put. I am merging clusters and timer which involves lots of callback functions. I put them into package "Callback".
hustfxj
changed the title
There was a problem hiding this comment.
I think this was supposed to be the leaderlockPath, that was not set.
There was a problem hiding this comment.
sorry, I made a mistake
|
Looks like you missed a reference to in-process-zookeeper (from the CI) |
There was a problem hiding this comment.
This is new code that didn't exist in storm proper. I think it would be cleaner to have this be a debug message instead of an info message. Also could you use "{}" substitution instead of +?
LOG.info("Zookeeper state update: {}, {}, {}", ZkKeeperStates.getStateName(state), ZkEventTypes.getStateName(type), path);
There was a problem hiding this comment.
Never mind about making it debug. I missed original implementation. I still would like to use the "{}" substitution though.
There was a problem hiding this comment.
The previous code never caught an exception, and especially did not eat it. Please either update the Interface to support Exceptions or wrap it in a RuntimeException and rethrow it.
|
I am done with my first pass through the code. Overall things look good, but there are a few corner cases around error recovery and running on Windows that need to be fixed before this can go in. Also please be sure to run all of the tests and that they all pass. |
|
Thanks @revans2 @wuchong @redsanket for all your thorough comments, I update the PR. |
There was a problem hiding this comment.
Not sure why we need this.
There was a problem hiding this comment.
OK I see that you missed updating zk-leader-elector.
There was a problem hiding this comment.
Again this is for stubbing, so if you want some help with this please let me know.
There was a problem hiding this comment.
yes， I don't know how to be better to port zk-leader-elector by java at nimbus_test.clj
|
Even with all of those changes to make the tests compile I am seeing test failures. Please make sure that passes before putting up a pull request. If you run into issues and cannot translate all of zookeeper.clj to java that is fine for a first pass others can help with different parts, but we want to be sure that we never break the build or it will take forever to stabilize later on. |
Conflicts: storm-core/src/clj/org/apache/storm/command/dev_zookeeper.clj storm-core/src/clj/org/apache/storm/command/shell_submission.clj storm-core/src/clj/org/apache/storm/testing.clj storm-core/test/clj/org/apache/storm/nimbus_test.clj STORM-1257: port backtype.storm.zookeeper to java
Upmerge and add ability to mock Zookeeper functions
There was a problem hiding this comment.
It's better to change the log message a little bit more descriptive.
|
@revans2 Thanks, I learned a lot about storm&clojure. |
|
The changes look good to me now and I am +1 on merging them in. But because I helped contribute some of the code I want at least one other committer to also give a +1. |
|
Very minor nit. Otherwise, +1 |
|
A line of white space is simple enough for me to do as I check it in. |
7 participants
