twitter / summingbird

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

Refactor `SummerBuilder` to make it possible to use `Counters` on `SummerBuilder` creation #738

Closed ttim closed 7 years ago

ttim commented 7 years ago

Currently the way to customize Summer instances is not perfect - we have an old 'legacy' way based on a lot of different options and 'new' way with setting SummerBuilder directly. At the same time with new way it's impossible to fully utilize Counters because jobId is needed to create them. Also new way is quite verbose.

This PR adds a way to use Counters in SummerBuilder and couple of SummerBuilders: Null, sync and async.

ttim commented 7 years ago

There is an issue for that: https://github.com/twitter/summingbird/issues/711

codecov-io commented 7 years ago

Codecov Report

Merging #738 into develop will decrease coverage by 0.13%. The diff coverage is 39.28%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #738      +/-   ##
===========================================
- Coverage    71.99%   71.86%   -0.14%     
===========================================
  Files          152      153       +1     
  Lines         3721     3735      +14     
  Branches       208      202       -6     
===========================================
+ Hits          2679     2684       +5     
- Misses        1042     1051       +9
Impacted Files Coverage Δ
...om/twitter/summingbird/online/option/AllOpts.scala 62.5% <0%> (-20.84%) :arrow_down:
...om/twitter/summingbird/online/option/Summers.scala 32.35% <32.35%> (ø)
...la/com/twitter/summingbird/storm/BuildSummer.scala 50% <55%> (+6.52%) :arrow_up:
.../twitter/summingbird/memory/ConcurrentMemory.scala 97.14% <0%> (+1.42%) :arrow_up:
.../main/scala/com/twitter/summingbird/Producer.scala 77.27% <0%> (+1.51%) :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 6f8dd8f...22532c0. Read the comment docs.

ttim commented 7 years ago

@johnynek I know here is quite a lot of code without test coverage, if you think it worth it to add tests on that - I can do this. But personally I think it's ok to leave this kind of code without tests.

ttim commented 7 years ago

@johnynek @pankajroark can you look again did I point all your comments? Thanks!

johnynek commented 7 years ago

👍