twitter / summingbird

Streaming MapReduce with Scalding and Storm
https://twitter.com/summingbird
Apache License 2.0
2.14k stars 267 forks source link

Fix summer options propagation #739

Closed ttim closed 7 years ago

ttim commented 7 years ago

Currently if you have flatMap().name('flatMap').sumByKey().name('sumByKey') and you have some Summer options on both flatMap and sumByKey for both of nodes (for partial aggregation on flatMap and for final aggregation on sumByKey) only options from flatMap will be applied.

Root cause of this is in a way we get options for Nodes - we get options for first producer in producers chain corresponding to Node. In case of SummerNode members contains two Producers - IdentityKeyedProducer and Summer.

This PR fixes that by passing Producer instance to BuildSummer which is used to get options for summer.

codecov-io commented 7 years ago

Codecov Report

Merging #739 into develop will increase coverage by 0.3%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #739     +/-   ##
==========================================
+ Coverage    71.88%   72.18%   +0.3%     
==========================================
  Files          153      153             
  Lines         3735     3736      +1     
  Branches       202      204      +2     
==========================================
+ Hits          2685     2697     +12     
+ Misses        1050     1039     -11
Impacted Files Coverage Δ
...n/scala/com/twitter/summingbird/planner/Node.scala 83.78% <100%> (+0.22%) :arrow_up:
...twitter/summingbird/storm/SummerBoltProvider.scala 100% <100%> (ø) :arrow_up:
...itter/summingbird/storm/StormTopologyBuilder.scala 94.44% <100%> (ø) :arrow_up:
...la/com/twitter/summingbird/storm/BuildSummer.scala 75% <100%> (+25%) :arrow_up:
.../com/twitter/summingbird/storm/SpoutProvider.scala 97.05% <100%> (ø) :arrow_up:
...witter/summingbird/storm/FlatMapBoltProvider.scala 100% <100%> (ø) :arrow_up:
...a/com/twitter/summingbird/storm/StormTestRun.scala 60% <0%> (-6.67%) :arrow_down:
...ain/scala/com/twitter/summingbird/TestGraphs.scala 87.89% <0%> (-0.64%) :arrow_down:
...om/twitter/summingbird/online/option/Summers.scala 55.88% <0%> (+23.52%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c272b2a...36cf778. Read the comment docs.

johnynek commented 7 years ago

👍