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 summingbird-storm compile with storm 0.10.0 #644

Closed johnynek closed 8 years ago

johnynek commented 8 years ago

Did this to address #643. Had to make a few changes due to slf4j changes that I guess get pulled in transitively (man, transitive deps are kind of a broken idea, I'm afraid, who knows what other versions get bumped with this change?).

The real issue is that storm 0.9.0-wip15 was using an old zookeeper that is incompatible with recent finagle, which is also used in the examples. The solution to #643 then is to upgrade storm to get on a compatible zookeeper.

ianoc commented 8 years ago

Shame so many seemingly unrelated changes are required, that said they look fine to me. I don't believe log4j logging is call by name though so moving to interpolation might make the logging more expensive i guess. But its all log.info we expect people to not be on performance critical paths anyway.

LGTM, merge when green

johnynek commented 8 years ago

@ianoc I think all the logging is out of the critical path (just startup/plan logging).

The change I made in FlatMapBoltProvider was just to kill a warning while I was at things.