STORM-386 nodejs multilang protocol implementation and examples

[anyatch]

More details at:
https://issues.apache.org/jira/browse/STORM-386

[ptgoetz]

Sorry for getting to this so late. This looks interesting.

I'll need to experiment before I approve, but I will definitely review it.

I'd be interested in hearing more about how you are using it.

[ptgoetz]

Also, could you file a JIRA for this and change the pull request title to include the JIRA number?

[itaifrenkel]

The JIRA is STORM-386 as mentioned in the description (will fix the title once the developer is back in the office). We are using it in production already to migrate nodejs code to storm (low throughput low latency application).

[revans2]

We generally let git keep track of the authorship of files instead of marking the files with owners.

[revans2]

The idea is definitely interesting. I have a few suggestions though.

The first is with the 'done' callback. I can see how it is needed to support async processing, but for me it feels like it is not likely to be too common, but then again I have not done anything significant with javascript and nothing with nodejs so I just don't know with the libraries are like.

My other issue is around packaging and testing. If we are going to officially support nodejs as an execution environment similar to how we support python and ruby I would like to see us running unit tests with it like we do with python and ruby. I would also like to see it packaged and distributed with storm, and not in the resources of storm-starter.

Alternatively we could set it up as a separate jar similar to the kafka spout, that would have the storm.js in the resources directory, and then ask users to add it as a maven dependency when packaging their own topologies. Either way though I would like to see us test this.

[itaifrenkel]

On your first comment:
Node.js APIs have their own standard for asynchronous functions:errors are passed as the first argument to the callback function. See for example http://nodejs.org/api/fs.html#fs_fs_readfile_filename_options_callback
Since the ISpout and IBolt methods are "void methods" the nodejs callback equivalent only receives one argument (error if exists) and does not receive a second argument (return value). Hence this method may sometimes be called "done" instead of "callback".
Another approach would be to use Promise objects (such as whenjs), however these means adding an opinionated dependency. Most promise libraries have "lift" method that converts standard nodejs async functions into their promise equivalents.
Another approach would be using the nodejs on event pattern, but we tried to stick to the python implementation as much as we could, so we haven not considered it.

[revans2]

Your logic seems fine to me. Like I said I am not super familiar with the nodejs APIs.

I would, however like to see tests and formal packaging for it. If we are going to support it, we should fully support it.

I also am curious how likely others would be to use this library too. Although it is a relatively small amount of code, it could become a maintenance issue.

[itaifrenkel]

Unit Tests: We can follow the way storm.rb is handled in the repository (3 copies of the same file) and translate the ruby unit tests into nodejs. I can see why you don't want a third window being broken. But I request to take the multilang cleanup into another JIRA, which leads me to ...

Packaging: Deploying and upgrading ruby/python/nodejs code is really different than Java code. Java code in storm is JARed-with-dependencies. Packing nodejs code with its entire npm dependency tree and automating it with maven is possible, but you loose the no-need-to-do-the-compile-thingy nature of the language. So even if we do call gem/pip/npm install from within the maven build process, pack everything in a JAR and ship it with storm, I am not sure I would use it that way. Instead the JAR would just be pure java code pointing to the folder where the ruby/python/nodejs code is. And that is also the folder where storm.rb/py/js would be copied to. So there is no reusable component you can ship with storm, other than the official storm.js file that was tested to work with this version of Storm.

Maintenance: The amount of maintenance of the multilang project is approx numberOfFeatures times the numberOfLanguages times the numberOfSerialization protocols.
IMO the numberOfLanguages is a strong selling point for Storm (choose the best language that fits your business). Both ruby/python/node have a big crowd of open-source developers this project can rally.
IMO numberOfFeautres that are not data related is not a strong selling point. For example I would not use the storm logging or monitoring through the multilang protocol. If there is a problem with the multilang/storm/whatever I would like a nodejs library to log it to a file, or contact directly the monitoring server, especially as Storm does the fail-fast-the-worker-with-all-child-processes each time a bolt/spout has an uncaught exception. I doubt the multilang logging could deliver some unexpected error in time before the stdout pipe is closed.
As for serialization protocols.... it was abstracted on the Java side recently which allows third party plugins to squeeze more CPU cycles with custom serialization protocols. That's nice, but as JSON is the only supported protocol by the ruby/python/nodejs implementations adding another serialization protocol requires a feature matrix. The proper thing to focus on Storm's core competency and that is Data. JSON would eventually become the bottle neck on the Worker juggling multiple subprocess via serialization and de-serialization of multilang messages. It takes approx 1% CPU core and 1ms latency per multilang process assuming 1 message per second (hardware dependent ofcourse). Instead of JSON I would pick one serialization protocol that has a heavily optimized Java implementation and also widely used python/node/ruby/etc implementations that hopefully do not require native code to install and stick to it. Use serialization/deserialization optimization on the java side (caching) for better performance of the nextTuple and sync messages.

[itaifrenkel]

Hi, the latest version is working for quite a while in production, and includes unit tests.

[revans2]

Sorry I have been fighting some fires and am trying to catch up on things now. I will try it out.

[revans2]

Great to hear that the latest code is in prod now.

I ran into a few problems, but none of them are really your code. The test didn't fail when I didn't have nodejs installed. but this is because of a bug in util.clj STORM-461

Also I really hate having two copies of storm.js in there, but we have the same thing for ruby and python. I files STORM-462 for this too.

The last problem I has was with documentation. We don't indicate anywhere what needs to be installed to build/run the tests. Could you add a section to DEVELOPER.md that indicates that you need ruby, python, and nodejs for all of the tests to pass.

I also assume that you are going to be able to help support this long term?

[d2r]

Indentation seems off here.

[d2r]

I am OK with adding this to multilang if others are OK. I agree we can move it later if need be.
I am trying to find out why multilang tests are not failing for me even when I do not have nodejs installed.

[d2r]

For testing, I think I need #231 first.

[d2r]

OK, after up-merging to master and pulling in #231, the tests failed with out nodejs installed and passed with nodejs installed.
I am +1, and we can fix license files and formatting comments when it is merged in.
I'll hold off for @harshach to comment

[harshach]

I've gone through the code and ran the build with node v0.10.28 installed on osx all the test passes. I ran a single node cluster and deployed WordCountTopologyNode and it stops after less than minute with this Spout errror
Emitting: spout __ack_init [1107591454335615211 4198602525173443000 24]
2014-09-02 10:51:42 b.s.util [ERROR] Async loop died!
java.lang.RuntimeException: java.lang.RuntimeException: backtype.storm.multilang.NoOutputException: Pipe to subprocess seems to be broken! No output read.
Serializer Exception:

apache-storm-0.9.3-incubating-SNAPSHOT/storm-local/supervisor/stormdist/wordcount-1-1409680231/resources/randomsentence.js
:59
self.log(this.pending[id] + ' sent to task ids - ' + taskIds);
^
ReferenceError: self is not defined

and my topology complete latency at 30757.945. I haven't debugged enough to find the cause.
I am able to run WordCountTopology without any issues on the same cluster. It might be related my environment (os x). I'll look into this to find more details. If anyone encountered similar issue or ran WordCountTopologyNode without any issues please let me know.

[anyatch]

Referring the ReferenceError - it was caused by a bug in randomsentence.js that happens only on case of a "fail" for one of the tuples. I fixed the bug (fix committed), still not sure why did you get "fail" in the split sentences example.

[harshach]

@anyatch Thanks. Will test the changes. The above error showed up because of shell process broken probably caused by randomsentence.js bug.

[harshach]

@anyatch I don't see the above mentioned issue anymore. But for some reason
complete_latency on topology ui goes up very high
window emitted transferred complete latency(ms) acked failed
10m 0s 747380 453520 24592.787 801800 8600
concerned with high complete latency and failed tuples and after a while topology stops
I understand that WordCountTopologyNode is an example but I think it should be working properly as users can refer to it as documentation towards writing a topology in node.js.
I'll try to find more details (new to node.js) this evening. If you can check why there are failed tuples and complete latency is high that will be great.

[harshach]

Looks like you are anchoring the tuple here but there is no ack in WordCountTopologyNode.WordCount Bolt. This might be the one causing the fail count go up and also completeLatency. I removed the anchorTupleId and in storm.js anchors as part of the message. There is no failed tuples and lower completeLatency of 41.

[itaifrenkel]

WordCountTopologyNode.WordCount Bolt extends BaseBasicBolt which is then wrapped by BasicBoltExecutor which AFAIK acks

[harshach]

RandomSentenceSpout adds a sleep(100) in nextTuple and its not in randomsentece.js that explains the high completeLatency

[harshach]

I am +1 on merging this as is and fixing those above mentioned issues as part of another jira. I don't see any issues with the api above small fixes in example topologies. Thanks.

[itaifrenkel]

@harshach Isn't it a better practice to set topology.sleep.spout.wait.strategy.time.ms to 100 instead of adding sleep to the spout ? I could add the equivalent of sleep(100) to randomSentence.js even so.

[harshach]

@itaifrenkel I agree topology.sleep.spout.wait.strategy.time.ms is a better option.

[harshach]

@ptgoetz @revans2 can we merge this PR.

[revans2]

Sorry this took so long to get in. +1 I just merged it to master. Not sure how your team wants to be recognized in the README? There are a lot of contributors to this, but if you want to put us a list of names I can add them in.

[itaifrenkel]

I just reviewed the merge. It seems like we placed storm.js in the wrong folder when compared to storm.py. Here are the correction steps:

mkdir storm-core/src/multilang/js
git mv storm-core/src/dev/resources/storm.js storm-core/src/multilang/js/storm.js
ln -s storm-core/src/multilang/js/storm.js storm-core/src/dev/resources/storm.js

[revans2]

@itaifrenkel

Could you file a new pull request for those changes? you can reopen the same JIRA if you like, or file a new one.

[itaifrenkel]

Opened #265

[itaifrenkel]

If you could please add @anyatch and @itaifrenkel to the readme that would be nice. thanks.

[revans2]

@itaifrenkel and @anyatch both of you should be in README.markdown now. Thanks again for your contribution.