twitter / summingbird

Streaming MapReduce with Scalding and Storm

https://twitter.com/summingbird

Apache License 2.0

2.14k stars 267 forks source link

Fix the bug with reducer estimators not working #714

Closed oscar-stripe closed 7 years ago

oscar-stripe commented 7 years ago

Addresses #713 by using the same code path scalding uses.

johnynek commented 7 years ago

the tests are getting intolerable. We really need to look into the flakiness. I think they fail more than 1/2 the time on travis.

codecov-io commented 7 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (develop@44fb5c9). Click here to learn what that means.

@@            Coverage Diff             @@
##             develop     #714   +/-   ##
==========================================
  Coverage           ?   71.01%           
==========================================
  Files              ?      141           
  Lines              ?     3471           
  Branches           ?      195           
==========================================
  Hits               ?     2465           
  Misses             ?     1006           
  Partials           ?        0

Impacted Files	Coverage Δ
...witter/summingbird/scalding/ScaldingPlatform.scala	`76.78% <100%> (ø)`

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 44fb5c9...78a026c. Read the comment docs.

johnynek commented 7 years ago

@pankajroark this should be ready to merge when you can review.

johnynek commented 7 years ago

Maybe @benpence would also be good to look at this.

benpence commented 7 years ago

+1 @johnynek nice work! I have a branch from several months ago to do something similar, but got distracted writing the test. This should also give us the flow(step) listeners for free right (I think you spoke about this yesterday)?

johnynek commented 7 years ago

Yes. This fixes that as well. On Fri, Feb 10, 2017 at 14:35 Ben Pence notifications@github.com wrote:

+1 @johnynek https://github.com/johnynek nice work! I have a branch from several months ago to do something similar, but got distracted writing the test. This should also give us the flow(step) listeners for free right (I think you spoke about this yesterday)?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/twitter/summingbird/pull/714#issuecomment-279103875, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdi1AMN6KwPCs27x51QHv9QBzZhDwks5rbQJfgaJpZM4L82pk .

johnynek commented 7 years ago

So we could remove the mutate stuff and implement that with Config flow listeners. On Fri, Feb 10, 2017 at 14:36 P. Oscar Boykin oscar.boykin@gmail.com wrote:

Yes. This fixes that as well. On Fri, Feb 10, 2017 at 14:35 Ben Pence notifications@github.com wrote:

+1 @johnynek https://github.com/johnynek nice work! I have a branch from several months ago to do something similar, but got distracted writing the test. This should also give us the flow(step) listeners for free right (I think you spoke about this yesterday)?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/twitter/summingbird/pull/714#issuecomment-279103875, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdi1AMN6KwPCs27x51QHv9QBzZhDwks5rbQJfgaJpZM4L82pk .

pankajroark commented 7 years ago

👍