twitter / summingbird

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

remove duplicate LookupJoin - use Scalding's. #654

Closed sritchie closed 8 years ago

sritchie commented 8 years ago

596 already did this, but that branch has merge conflicts. Deletes our implementation and tests and relies on the Scalding version of LookupJoin.

The dependencies mentioned in #596 are all complete.

r? @johnynek

ianoc commented 8 years ago

Looks like build is failing?

[info] - should compute correct statistics *** FAILED ***
[info]   java.lang.NullPointerException:
[info]   at com.twitter.summingbird.scalding.ScaldingLaws$$anonfun$1$$anonfun$apply$mcV$sp$12.apply$mcV$sp(ScaldingLaws.scala:605)
[info]   at com.twitter.summingbird.scalding.ScaldingLaws$$anonfun$1$$anonfun$apply$mcV$sp$12.apply(ScaldingLaws.scala:573)
[info]   at com.twitter.summingbird.scalding.ScaldingLaws$$anonfun$1$$anonfun$apply$mcV$sp$12.apply(ScaldingLaws.scala:573)
[info]   at org.scalatest.Transformer$$anonfun$apply$1.apply$mcV$sp(Transformer.scala:22)
[info]   at org.scalatest.OutcomeOf$class.outcomeOf(OutcomeOf.scala:85)
[info]   at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
[info]   at org.scalatest.Transformer.apply(Transformer.scala:22)
[info]   at org.scalatest.Transformer.apply(Transformer.scala:20)
[info]   at org.scalatest.WordSpecLike$$anon$1.apply(WordSpecLike.scala:953)
[info]   at org.scalatest.Suite$class.withFixture(Suite.scala:1122)
johnynek commented 8 years ago

weird. This is the line with the NPE:

https://github.com/twitter/summingbird/blob/develop/summingbird-scalding-test/src/test/scala/com/twitter/summingbird/scalding/ScaldingLaws.scala#L605

It looks like that should only happen if the job does not run. Restated.

sritchie commented 8 years ago

Strange. @johnynek @ianoc merge or investigate more?

ianoc commented 8 years ago

If its green I think we are good to go. In that maybe we want to check sometime if this re-occurs, but seems unrelated to change