[STORM-138]: Pluggable serialization for multilang #84
Conversation
…ialiser being used. Moved multilang classes to multilang directory.
…es. Updated dependencies.
…d Immission to ShellMsg and BoltMsg respectively. Documented ISerializer interface.
…d Immission to ShellMsg and BoltMsg respectively. Documented ISerializer interface.
Conflicts: conf/defaults.yaml
jsgilmore
changed the title
|
Hi @jsgilmore, are there any tests that would help to validate the change? It is unfortunate the old PR is not accessible. @xumingming, can you comment? |
|
Hi @d2r. The pull request is a refactoring of the original multilang protocol. The previous multilang unit tests are still sufficient to test whether the refactoring was successful. No features were added. |
|
@d2r, here's the original pull request with @xumingming's comments: https://github.com/nathanmarz/storm/pull/697/files I'll try to review this today as well. |
|
From a quick pass through the code the core of it looks good. I just have one major concern. The way to select the serialization method appears to be a cluster wide configuration, but seems to be read from the topology configuration. At a minimum the configuration should begin with "topology." but I would really prefer to have it be something that subclasses to ShellSpout and ShellBolt set. I can easily see a topology where some ShellBolts are using JSON, and others are using a custom binary protocol. Having it be topology wide seems too restrictive. |
|
We have not seen the need to have different encoding schemes on a bolt level. I can't see why you would want to use JSON at all if a scheme is available that provides better throughput at lower CPU requirements. We moved from JSON to protocol buffers and now all our topologies use that scheme. It would be helpful to get some more thoughts on this subject and if it is required, I would be happy to change it, but I would prefer to do it as a future pull request. I don't mind changing the configuration option name. |
|
I agree that a PB impl would be superior to the JSON, but I am thinking about my customers who have legacy shell bolts already. I can see some of them wanting to keep what already works in place and just add new bolts with the new protocol until they find the time to go back and upgrade. I am fine with having the change be done in a follow up JIRA. |
|
At the very least, this should be a topology-specific configuration. |
|
Topology specific configuration generally just start with "topology." that distinguishes them from other configurations for nimbus, supervisors, or globally. |
|
The multilang configuration is now topology specific. |
|
The code looks good to me and all of the unit tests pass, I am +1 |
|
|
||
| String stream = (String) msg.get("stream"); | ||
| if (stream == null) | ||
| stream = Utils.DEFAULT_STREAM_ID; |
There was a problem hiding this comment.
Sorry, but I don't understand what you mean with "block" here? Do you mean functional block {}?
There was a problem hiding this comment.
Yes, that's what I meant. Not a big deal though.
|
Just had some minor questions/comments. Looks good. |
|
+1 |
|
+1 |
…ullRequest class. Tests cases were also updated. For PullRequestTest#fetch(), apache/storm/pull/84 pull request was used.
…ullRequest class. Tests cases were also updated. For PullRequestTest#fetch(), apache/storm/pull/84 pull request was used.
Simplify checking for unreadable config file for jdk7
BUG-51669: Allowing users to specify the nimbus thrift server queue s…
This pull request addresses storm issue STORM-138 (https://issues.apache.org/jira/browse/STORM-138) and makes the Storm multilang JSON protocol plugable. This pull request was originally reviewed and accepted by James Xu.