Add Window Bolt support #1215

windie · 2016-08-04T05:43:31Z

This adds Window bolt support. Most of codes are from Apache Storm master branch (https://github.com/apache/storm/tree/v1.0.2).

This only includes minimum codes for Windowed Bolt. I plan to add others in follow-up PRs.

Resolves #1124.

windie · 2016-08-04T05:51:51Z

@kramasamy - right now most of codes are from Strom's master branch. I'm not sure if this is the right base. Or should I use some stable version, such as v1.0.2 or v1.1.0? In addition, is there anything I can do to make this one easy to review?

kramasamy · 2016-08-04T05:54:13Z

@windie - I would suggest to use a stable version such as v1.0.1, then when we upgrade we can do the next version - it is very predictable.

windie · 2016-08-04T06:02:01Z

@kramasamy sounds great. Will update my PR to base on v1.0.1.

kramasamy · 2016-08-04T06:02:46Z

thanks @windie - really appreciate the contribution.

kramasamy · 2016-08-04T06:07:04Z

@windie - we can add you into our slack group of heron committers. But I don't know your email address.

windie · 2016-08-04T06:20:56Z

@kramasamy - Thanks! My email is ******

kramasamy · 2016-08-04T06:22:42Z

Invited - please remove your email from the comment.

kramasamy · 2016-08-06T19:01:27Z

@windie - this should go into heron/storm - since this is storm api 1.0.0 compatible. Heron api under heron/api - is not supposed to be used by the users - since it is a hidden API.

windie · 2016-08-08T05:39:26Z

@kramasamy － Gotcha.

windie · 2016-08-15T07:29:59Z

The new files are copied from Storm. I created a diff between them to show the differences: https://gist.github.com/windie/28d84a7b00a587cd242ba55681df56dd

windie · 2016-08-15T07:31:32Z

heron/storm/src/java/org/apache/storm/task/TopologyContext.java

@@ -186,6 +190,14 @@ public int getThisTaskIndex() {
        return getSources(getThisComponentId());
    }
  */
+  public Set<GlobalStreamId> getThisSourceIds() {


I'm not sure if this is the correct way.

Right now Heron doesn't have the Grouping class, so I created the method to get all source streams. It's used by WindowedBoltExecutor.

In Storm, WindowedBoltExecutor uses getThisSources (The above method commented out)

windie · 2016-08-15T07:32:06Z

@kramasamy － updated. Please take a look.

windie · 2016-08-16T03:04:03Z

I fixed the failure test in #1263

kramasamy · 2016-08-17T06:49:31Z

@nlu90 @objmagic - can you co-review the PR and discuss the design?

objmagic · 2016-08-17T06:51:02Z

@kramasamy will do when we have time...

kramasamy · 2016-08-26T06:47:39Z

@maosongfu - can you review this?

kramasamy · 2016-10-07T04:09:48Z

@windie - can you please add some detailed documentation for the types of windows supported? We want to verify and close this as soon as possible.

kramasamy · 2016-10-07T04:16:37Z

@windie - how can we write unit tests for this?

mycFelix · 2016-10-07T12:44:31Z

@windie - Hi, so great to add Window Bolt support!
I checkouted PR and installed heron-storm on my local env. Then I run test-topology named SlidingWindowTopology given by Apache-Storm. The only thing I have changed in SlidingWindowTopology.java is following

 Config conf = new Config();
        conf.setDebug(true);
//        if (args != null && args.length > 0) {
//            conf.setNumWorkers(1);
//            StormSubmitter.submitTopologyWithProgressBar(args[0], conf, builder.createTopology());
//        } else {
//            LocalCluster cluster = new LocalCluster();
//            cluster.submitTopology("test", conf, builder.createTopology());
//            Utils.sleep(40000);
//            cluster.killTopology("test");
//            cluster.shutdown();
//        }

 StormSubmitter.submitTopology("test", conf, builder.createTopology());

I failed to run test-topology. Would you please take a look? And The error log is following:

[2016-10-07 20:19:04 +0800] com.twitter.heron.instance.HeronInstance ERROR:  Exception caught in thread: SlaveThread with id: 13 
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Number
    at org.apache.storm.topology.WindowedBoltExecutor.initWindowManager(WindowedBoltExecutor.java:153)
    at org.apache.storm.topology.WindowedBoltExecutor.prepare(WindowedBoltExecutor.java:267)
    at org.apache.storm.topology.IRichBoltDelegate.prepare(IRichBoltDelegate.java:48)
    at com.twitter.heron.instance.bolt.BoltInstance.start(BoltInstance.java:110)
    at com.twitter.heron.instance.Slave.startInstance(Slave.java:173)
    at com.twitter.heron.instance.Slave.handleNewAssignment(Slave.java:159)
    at com.twitter.heron.instance.Slave.access$200(Slave.java:40)
    at com.twitter.heron.instance.Slave$1.run(Slave.java:85)
    at com.twitter.heron.common.basics.WakeableLooper.executeTasksOnWakeup(WakeableLooper.java:142)
    at com.twitter.heron.common.basics.WakeableLooper.runOnce(WakeableLooper.java:74)
    at com.twitter.heron.common.basics.WakeableLooper.loop(WakeableLooper.java:64)
    at com.twitter.heron.instance.Slave.run(Slave.java:169)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

I have a minor comment though. I think it will be great if we can add unit tests,example topology and doc for this.

mycFelix · 2016-10-08T01:34:46Z

heron/storm/src/java/org/apache/storm/topology/WindowedBoltExecutor.java

+
+  private Set<GlobalStreamId> getComponentStreams(TopologyContext context) {
+    Set<GlobalStreamId> streams = new HashSet<>();
+    for (GlobalStreamId streamId : context.getThisSourceIds()) {


~~I was wondering if we should check streamId as what storm has done here. I'm afraid it will cause another problem at line. What do you think?~~

my bad, delete comment

windie · 2016-10-08T05:04:28Z

Thanks for reviewing. I will add examples, docs and unit tests at the weekend.

mycFelix · 2016-10-20T05:41:33Z

@windie - hi, any update on this?

kramasamy · 2016-10-24T18:45:56Z

@windie - any update on this?

windie · 2016-10-25T07:24:57Z

Sorry for the delay. Just added examples and unit tests to this PR.

Right now this PR supports Storm's IWindowedBolt and Watermark but not IStatefulWindowedBolt.

IWindowedBolt example: com.twitter.heron.examples.SlidingWindowTopology.
Watermark example: com.twitter.heron.examples.SlidingTupleTsTopology.

windie · 2016-10-25T07:26:27Z

[2016-10-07 20:19:04 +0800] com.twitter.heron.instance.HeronInstance ERROR: Exception caught in thread: SlaveThread with id: 13
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Number

@mycFelix fixed this error in my PR. You can just run com.twitter.heron.examples.SlidingWindowTopology and com.twitter.heron.examples.SlidingTupleTsTopology in this PR.

mycFelix · 2016-10-25T07:29:30Z

@windie - Great! I'll check it ASAP. BTW, free to ping me at SLACK group anytime.

Fix the style erorrs

billonahill · 2016-10-25T17:35:05Z

heron/examples/src/java/com/twitter/heron/examples/RandomIntegerSpout.java

+ * Emits a random integer and a timestamp value (offset by one day),
+ * every 100 ms. The ts field can be used in tuple time based windowing.
+ */
+@SuppressWarnings("serial")


Instead of suppressing this here and elsewhere, can you add a serialVersionId?

billonahill · 2016-10-25T17:45:16Z

If the storm code is mostly copied could you add a README.md file below heron/storm/src that explains what storm version we're using and how to diff against the storm repo to pull in future changes?

mycFelix · 2016-10-26T06:59:49Z

The bug mentioned before is fixed. 👍

windie · 2016-10-27T03:34:32Z

What's the convention for codes copied from Storm? Most of codes are changed because of the style errors and different log codes, it's impossible to apply any patches from storm repo directly.

maosongfu · 2016-10-27T03:41:11Z

@kramasamy @nlu90 @windie I will propose to put all of these on top of heron in another repo or in a folder exclude style checking.

windie · 2016-10-27T04:44:08Z

How about just excluding style checking for //heron/storm?

billonahill · 2016-10-27T05:10:37Z

+1 for excluding style checks for /heron/storm and syncing source code without style changes. This will make it easier to diff/patch withe the storm repo.

kramasamy · 2016-10-27T05:11:45Z

ok sounds good. Can we do separate PR for excluding style checking?

maosongfu · 2016-10-27T05:16:11Z

@windie @kramasamy @billonahill It may not be a good idea to exclude everything in /heron/storm since there are still codes owned by us in this folder, especially for the parts converting storm into heron. Is it possible in the separate PR that we split this folder into two: one is for codes owned by us enforcing style checks, while another is for codes from outside excluding style checks.

kramasamy · 2016-10-27T05:24:05Z

@maosongfu - any idea which directories under heron/storm is written by us? I can take care of separating it out.

maosongfu · 2016-10-27T05:40:03Z

@kramasamy
Currently these are written by us:
heron/heron/storm/src/java/

backtype/storm
org/apache/storm

They are converting storm job into heron job.

mycFelix · 2016-10-27T05:52:10Z

Hi,guys.
BTW, how do we deal with //contrib folder if it was copied from Apache-storm.

windie changed the title ~~Add Window Bolt support~~ [WIP] Add Window Bolt support Aug 4, 2016

objmagic added the new feature label Aug 4, 2016

objmagic assigned windie Aug 4, 2016

windie force-pushed the window branch from e90e878 to 294226c Compare August 15, 2016 07:27

windie changed the title ~~[WIP] Add Window Bolt support~~ Add Window Bolt support Aug 15, 2016

windie reviewed Aug 15, 2016
View reviewed changes

windie force-pushed the window branch from 435ce5a to 19ea783 Compare August 16, 2016 05:14

mycFelix reviewed Oct 8, 2016

View reviewed changes

Add basic Window support based on Storm v-1.0.2

ea960c2

Add examples and unit tests

4abe8ce

windie force-pushed the window branch from 19ea783 to 4abe8ce Compare October 25, 2016 07:20

Fix the compile error

37fbb36

Fix the style erorrs

windie force-pushed the window branch from 39c3067 to 37fbb36 Compare October 25, 2016 09:33

billonahill reviewed Oct 25, 2016

View reviewed changes

Add Window Bolt support #1215

Are you sure you want to change the base?

Add Window Bolt support #1215

Conversation

windie commented Aug 4, 2016 • edited Loading

windie commented Aug 4, 2016

kramasamy commented Aug 4, 2016 • edited Loading

windie commented Aug 4, 2016

kramasamy commented Aug 4, 2016

kramasamy commented Aug 4, 2016 • edited Loading

windie commented Aug 4, 2016 • edited Loading

kramasamy commented Aug 4, 2016

kramasamy commented Aug 6, 2016

windie commented Aug 8, 2016

windie commented Aug 15, 2016

windie Aug 15, 2016 • edited Loading

Choose a reason for hiding this comment

windie commented Aug 15, 2016

windie commented Aug 16, 2016

kramasamy commented Aug 17, 2016

objmagic commented Aug 17, 2016

kramasamy commented Aug 26, 2016

kramasamy commented Oct 7, 2016

kramasamy commented Oct 7, 2016

mycFelix commented Oct 7, 2016 • edited Loading

mycFelix Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

windie commented Oct 8, 2016

mycFelix commented Oct 20, 2016

kramasamy commented Oct 24, 2016

windie commented Oct 25, 2016

windie commented Oct 25, 2016

mycFelix commented Oct 25, 2016 • edited Loading

billonahill Oct 25, 2016

Choose a reason for hiding this comment

billonahill commented Oct 25, 2016

mycFelix commented Oct 26, 2016 • edited Loading

windie commented Oct 27, 2016

maosongfu commented Oct 27, 2016

windie commented Oct 27, 2016

billonahill commented Oct 27, 2016

kramasamy commented Oct 27, 2016

maosongfu commented Oct 27, 2016 • edited Loading

kramasamy commented Oct 27, 2016

maosongfu commented Oct 27, 2016

mycFelix commented Oct 27, 2016

windie commented Aug 4, 2016 •

edited

Loading

kramasamy commented Aug 4, 2016 •

edited

Loading

kramasamy commented Aug 4, 2016 •

edited

Loading

windie commented Aug 4, 2016 •

edited

Loading

windie Aug 15, 2016 •

edited

Loading

mycFelix commented Oct 7, 2016 •

edited

Loading

mycFelix Oct 8, 2016 •

edited

Loading

mycFelix commented Oct 25, 2016 •

edited

Loading

mycFelix commented Oct 26, 2016 •

edited

Loading

maosongfu commented Oct 27, 2016 •

edited

Loading