Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Fix summer options propagation #739

Merged
merged 4 commits into from
Jul 5, 2017
Merged

Conversation

ttim
Copy link
Collaborator

@ttim ttim commented Jul 1, 2017

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.

Copy link
Contributor

@pankajroark pankajroark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the description of the PR to indicate how including IdentityKeyedProducer in options calculation is an issue and why removing that is the right fix for this issue.

Options.getFirst[T](options, stormDag.producerToPriorityNames(producer))
}
node.members
.find(!_.isInstanceOf[IdentityKeyedProducer[_, _, _]])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we comment here why this is required?

Copy link
Collaborator Author

@ttim ttim Jul 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed a way I fixed the issue. Turned out way with ignoring IdentityKeyedProducers caused more bugs than fixed.

@codecov-io
Copy link

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%) ⬆️
...twitter/summingbird/storm/SummerBoltProvider.scala 100% <100%> (ø) ⬆️
...itter/summingbird/storm/StormTopologyBuilder.scala 94.44% <100%> (ø) ⬆️
...la/com/twitter/summingbird/storm/BuildSummer.scala 75% <100%> (+25%) ⬆️
.../com/twitter/summingbird/storm/SpoutProvider.scala 97.05% <100%> (ø) ⬆️
...witter/summingbird/storm/FlatMapBoltProvider.scala 100% <100%> (ø) ⬆️
...a/com/twitter/summingbird/storm/StormTestRun.scala 60% <0%> (-6.67%) ⬇️
...ain/scala/com/twitter/summingbird/TestGraphs.scala 87.89% <0%> (-0.64%) ⬇️
...om/twitter/summingbird/online/option/Summers.scala 55.88% <0%> (+23.52%) ⬆️

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
Copy link
Collaborator

johnynek commented Jul 3, 2017

👍

@ttim ttim merged commit fecdfa3 into twitter:develop Jul 5, 2017
@ttim ttim deleted the fix_summer_opts branch July 6, 2017 00:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants