search

twitter / summingbird

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

Clean up all but the transient warnings #708

Closed johnynek closed 7 years ago

johnynek commented 7 years ago

I removed all but warnings like this:

[warn] /Users/oscar/oss/summingbird/summingbird-scalding/src/main/scala/com/twitter/summingbird/scalding/store/VersionedBatchStore.scala:108: no valid targets for annotation on value injection - it is discarded unused. You may specify targets with meta-annotations, e.g. @(transient @param)
[warn]   implicit @transient injection: Injection[(K2, V2), (Array[Byte], Array[Byte])], override val ordering: Ordering[K])

it seems like this pattern that we have been cargo-culting may not be actually doing anything, but I didn't want to take the risk (yet, but we should fix it).

All the other seemed easy to fix without doing much change to the code.

I also disabled scalariform, which is annoying in that it changes the code under you. I think we should set up scalafmt as a CI check and a command in the build to get code formatting.

johnynek commented 7 years ago

Find and replace gone wrong. On Thu, Jan 12, 2017 at 15:52 Piyush Narang notifications@github.com wrote:

@piyushnarang approved this pull request.

Looks good to me but not super familiar with the SB code, so would be nice to get Pankaj's comments too.

In summingbird-scalding/src/main/scala/com/twitter/summingbird/scalding/batch/BatchedStore.scala https://github.com/twitter/summingbird/pull/708#pullrequestreview-16496271 :

 capturedBatcher: Batcher,
  • commutativity: Commutativity): TypedPipe[(LTuple2[K1, BatchID], (Timestamp, V))] = {
  • implicit val timeValueSemigroup: Semigroup[(Timestamp, V)] =
  • IteratorSums.optimizedPairSemigroupTimestamp, V
  • commutativity: Commutativity): TypedPipe[(LTuple2[K1, BatchID], (Timestamp, V1))] = {
  • implicit val timeV1alueSemigroup: Semigroup[(Timestamp, V1)] =

timeV1alueSemigroup, rename to timeV1Semigroup?

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

codecov-io commented 7 years ago

Current coverage is 71.02% (diff: 83.01%)

Merging #708 into develop will decrease coverage by 0.07%

@@            develop       #708   diff @@
==========================================
  Files           141        141          
  Lines          3461       3469     +8   
  Methods        3263       3274    +11   
  Messages          0          0          
  Branches        198        195     -3   
==========================================
+ Hits           2461       2464     +3   
- Misses         1000       1005     +5   
  Partials          0          0

Powered by Codecov. Last update db801c2...9077b28

johnynek commented 7 years ago

@pankajroark good to go with the new name?

pankajroark commented 7 years ago

👍

johnynek commented 7 years ago

the 2.10 build keeps timing out. Not really sure what is going on. I'm hoping the #699 will just fix this.

I can't see how changing these warnings can possibly be related.

ttim commented 7 years ago

@johnynek It's green now, I think it's better to merge this change before Storm's one.

johnynek commented 7 years ago

@ttim maybe some issue with travis? Maybe just flakey tests.