Change FutureCollector signature to fix https://github.com/twitter/st… #293
Conversation
| @@ -19,24 +19,24 @@ package com.twitter.storehaus | |||
| import com.twitter.util.Future | |||
|
|
|||
| /** A type to represent how Seq of futures are collected into a future of Seq[T] */ | |||
| trait FutureCollector[-T] extends java.io.Serializable { | |||
| def apply[T1<:T](in: Seq[Future[T1]]): Future[Seq[T1]] | |||
| trait FutureCollector extends java.io.Serializable { self => | |||
There was a problem hiding this comment.
do we need the self? I dont see how?
There was a problem hiding this comment.
My bad. I was trying something else earlier and forgot to remove the self. Removed.
|
HBase store is failing in these. Could it be related? |
|
I re-ran the tests and this time only the MutableTTLCacheProperties failed and only for scala 2.11.7. I think the HBase test is also flaky. |
|
can you comment out this flakey test and add an issue to fix it? |
…ffort. As it was before it would have thrown an exception on keys filtered by bestEffort collector.
|
I'm not that familiar with this, but LGTM. Is there any extra testing we can / should do on our side to validate this? |
|
Well, @pankajroark is on your side right? :) |
|
FYI: Change in FutureCollector signature requires changes in some exposed APIs in summingbird (and thus some user code as well). I suppose I'll need to make a release of storehaus and then create the PR in summingbird with the code changes and version upgrade of storehaus. |
|
👍 |
|
@johnynek that was a question for @pankajroark :) |
|
I ran e2e tests on our end and they look good. Merging this now. |
|
👍 good work. |
| @@ -67,3 +71,4 @@ with DefaultHBaseCluster[Store[String, String]] { | |||
| val store=HBaseStringStore(quorumNames, table, columnFamily, column, createTable,pool,conf,4) | |||
| store.convert[Long,Long](_.toString) | |||
| } | |||
| */ | |||
There was a problem hiding this comment.
oops, I didn't see this. I'm fixing those in #300
Fix for #292 by changing signature of FutureCollector.
Please note the change in MergeableStoreViaGetPut to use the supplied future collector instead of the implicit one.