search

twitter / summingbird

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

Make sure the tail in TPNamedProducer actually names a tail #706

Closed johnynek closed 7 years ago

johnynek commented 7 years ago

@pankajroark look good?

pankajroark commented 7 years ago

:thumbsup:

johnynek commented 7 years ago

well our terrible tests strike again.

This PR passes tests locally for me, but we are see OOM it looks like. This is such a trivial change, it if compiles, it almost has to be correct. Really hard to see how it could really cost more memory.

We need to really figure something out here.

pankajroark commented 7 years ago

Mima is also failing btw

johnynek commented 7 years ago

Oh yeah. Of course this takes a different type but actually the previous type was a bug (and hopefully on one actually created one of those themselves). On Wed, Jan 11, 2017 at 18:06 Pankaj Gupta notifications@github.com wrote:

Mima is also failing btw

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/twitter/summingbird/pull/706#issuecomment-272071084, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdsjIrdMC2JowvW0Wnb7ZFQJ54uh9ks5rRaaygaJpZM4LhLGL .

pankajroark commented 7 years ago

Yeah, the type change looks ok.

johnynek commented 7 years ago

our tests are so terrible.

Maybe when we upgrade storm we can finally have less flakiness.

We also need to suppress logs in the test output. Somehow the code we have for that isn't working.