search

twitter / storehaus

Storehaus is a library that makes it easy to work with asynchronous key value stores
Apache License 2.0
464 stars 86 forks source link

Change FutureCollector signature to fix https://github.com/twitter/st… #293

Closed pankajroark closed 8 years ago

pankajroark commented 8 years ago

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.

johnynek commented 8 years ago

HBase store is failing in these.

Could it be related?

pankajroark commented 8 years ago

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. https://travis-ci.org/twitter/storehaus

johnynek commented 8 years ago

can you comment out this flakey test and add an issue to fix it?

pankajroark commented 8 years ago

Commented out the flaky tests: HBaseStringStoreProperties and MutableTTLCacheProperties. There was already an issue for MutableTTLCacheProperties https://github.com/twitter/storehaus/issues/291. Created one for HBaseStringStoreProperties https://github.com/twitter/storehaus/issues/294

johnynek commented 8 years ago

merge when green. Added a few more issues: #296 #295

isnotinvain commented 8 years ago

I'm not that familiar with this, but LGTM. Is there any extra testing we can / should do on our side to validate this?

johnynek commented 8 years ago

Well, @pankajroark is on your side right? :)

pankajroark commented 8 years ago

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. For further testing I'm planning to run E2E tests on our side.

johnynek commented 8 years ago

👍

isnotinvain commented 8 years ago

@johnynek that was a question for @pankajroark :)

pankajroark commented 8 years ago

I ran e2e tests on our end and they look good. Merging this now.

johnynek commented 8 years ago

👍 good work.