storm platform sometimes makes nodes with only merges

[johnynek]

in some cases, we see storm nodes that only have merges, then it finally fans out.

Something like:

val src1 = ...
val src2 = ...
val merged = src1 ++ src2
val sink1 = merged.sumByKey(store1)
val sink2 = merged.sumByKey(store2)

Things like this have created a node that just has the merge. There is no need to do this. Storm could just subscribe to all the dependencies of the "all-merge" node.

If there is a cheap fix to this, it would be great, because we have some giant topologies that have something like 5 of these.

cc @ianoc

[ianoc]

From https://github.com/twitter/summingbird/blob/develop/summingbird-online/src/main/scala/com/twitter/summingbird/planner/OnlinePlan.scala#L194

this looks like its not supported i think/is the expected behavior. Thats probably the location to fix too

[ianoc]

(i think conceptually what you have requires creating multiple merge nodes, I don't think we generated any new nodes in planning before which is probably what this hits)

[johnynek]

I think we could allow merge nodes to be in multiple physical nodes.

On Fri, Sep 15, 2017 at 09:03 ianoc ***@***.***> wrote: (i think conceptually what you have requires creating multiple merge nodes, I don't think we generated any new nodes in planning before which is probably what this hits) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#749 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdpOqBS1iHHgr6QtuwJ-cbz_d4EpWks5sisoQgaJpZM4PZVBo> .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

[ianoc]

Yep I think that's the likely fix here. Some tests/rules will need special casing but it does seem like logically what we want to do. (This applies to flat maps too I suspect when they are interacting with merge nodes. But I think the tests will shake some of this out) On Sat, Sep 16, 2017 at 10:42 AM P. Oscar Boykin <notifications@github.com> wrote:

I think we could allow merge nodes to be in multiple physical nodes. On Fri, Sep 15, 2017 at 09:03 ianoc ***@***.***> wrote: > (i think conceptually what you have requires creating multiple merge > nodes, I don't think we generated any new nodes in planning before which is > probably what this hits) > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < #749 (comment)>, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAEJdpOqBS1iHHgr6QtuwJ-cbz_d4EpWks5sisoQgaJpZM4PZVBo > > . > -- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#749 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbQvGdUIot3352KkFnIqPM3kOG2wPjeks5sjAhagaJpZM4PZVBo> .

-- Sent from Gmail Mobile