search

agemooij / riak-scala-client

An easy to use, non-blocking, fast Scala client library for interacting with Riak.
http://riak.scalapenos.com/
Apache License 2.0
84 stars 24 forks source link

New fetchWithSiblings RiakBucket operation #36

Closed ajantis closed 9 years ago

ajantis commented 9 years ago

The idea behind a new 'fetchWithSiblings' operation is to give some more control to the user on how to deal with Riak siblings. ConflictResolver is a neat functionality but sometimes user might need even more control.

For example, here is a sample scenario: User wants to read a Riak entry first before update and check if there are any siblings already. If there are siblings then he might count them and stop performing further update operation if there are too many (according to certain threshold). Such 'peek-before-store' strategy could be useful when dealing with 'Siblings explosion' cases, and we found ourselves implementing similar thing in our project recently.

Best regards, Dmitry

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-1.23%) to 80.32% when pulling 37e907b5fec5331dddee368541cf1dcad33ea865 on ajantis:add-fetch-with-siblings-operation into 657fa807f998b498d4578a4957871a0967ac3e9a on agemooij:master.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.58%) to 80.97% when pulling d0c120cc915a316d49a864a7053170f3389270ba on ajantis:add-fetch-with-siblings-operation into 657fa807f998b498d4578a4957871a0967ac3e9a on agemooij:master.

ajantis commented 9 years ago

I believe now coverage is still decreased because a test for

case other           ⇒ throw new BucketOperationFailed(...)

is missing. Although it's a bit tricky to test.

agemooij commented 9 years ago

Thanks Dimitry. I'll look at it ASAP. Busy busy as usual :cry:

ajantis commented 9 years ago

Hi Age! OK, thanks! No rush. ;)

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.58%) to 80.97% when pulling 7fa8a682e49a0f0c35238618978b4a5a15a521f0 on ajantis:add-fetch-with-siblings-operation into 657fa807f998b498d4578a4957871a0967ac3e9a on agemooij:master.

ajantis commented 9 years ago

Hi! Is there something I can do to help to merge that PR? :)

I see that coverage metrics had failed a bit by 0.58%. That's, I believe, because not all cases are covered with the test. In particular, error response from Riak. Other tests in the same spec also don't cover this case (the one that leads to throwing a BucketOperationFailed exception). However I can try to add some sort of mocking that this type of response, but it will need a bit of refactoring for the whole test case suite.

Best regards, Dmitry

agemooij commented 9 years ago

Sorry Dmitry, I just forgot about this one. I reviewed the PR with a few small comments. If you fix them I can merge ASAP.

Would you like a new release to be made with this change?

On a side note. Perhaps you'd like to become a committer and take over the maintenance of this project? I haven't touched Riak in years and I'm clearly not doing a very good job of maintaining it myself. It should probably be upgraded to the latest Scala/Spray/Akka/etc dependencies too.

ajantis commented 9 years ago

Hi Age!

Sorry Dmitry, I just forgot about this one. I reviewed the PR with a few small comments. If you fix them can merge ASAP.

Thanks a lot for the review! I applied the comments. Although the 'ignoreTombstones' flag is not covered with tests yet (you can check my comment there).

Would you like a new release to be made with this change?

Yes, that would be great!

On a side note. Perhaps you'd like to become a committer and take over the maintenance of this project? I haven't touched Riak in years and I'm clearly not doing a very good job of maintaining it myself. It should probably be upgraded to the latest Scala/Spray/Akka/etc dependencies too.

Sure, I would be glad to step up as a committer. :) We are still using this library in NavCloud and pretty happy with it. Maybe we can also contribute new features, like API support of CRDTs for Riak 2.0+.

And upgrading dependencies is definitely needed, yep. We actually did that in a sort of a nasty way, by rebuilding it with a specific combination of Akka and Spray that we were using at some point, and publishing it to intranet repository. But ideally, of course, is to use only official builds from central repository.

Cheers, Dmitry

agemooij commented 9 years ago

Awesome! I made you an official collaborator so you now have push access.

I'll have to check the release setup since that currently uses Maven Central directly instead of the less restrictive Bintray. One step at the time :smile:

agemooij commented 9 years ago

So now you should be able to upgrade all the dependencies to their latest values. I'm be offline for 4 weeks starting this coming weekend so if you want a release it's now or never I guess :smile:

ajantis commented 9 years ago

Thanks Age!

Shall I merge this one then?

I also created a PR with dependencies update, but it turned out to be only minor versions upgrade. i.e. 'nice to have', but not critical. :) https://github.com/agemooij/riak-scala-client/pull/37 (Want to run through the CI first before pushing directly).

So now you should be able to upgrade all the dependencies to their latest values. I'm be offline for 4 weeks starting this coming weekend so if you want a release it's now or never I guess :smile:

Oh. Ok :) Have a good holiday then! I'm actually also on holiday since today. :) It's not critical for us to have a new version released right now (for this particular PR we have a workaround in our codebase for a time being anyway). So if you cannot make a release before going on vacation - no problem, we can wait.

Cheers, Dmitry

agemooij commented 9 years ago

Yo, I won't have time to do anything before I'm off I'm afraid so good to know that there is no big rush. You can merge the PR if its up to me.

Talk to you when we're both back then. Enjoy your holidays!

ajantis commented 9 years ago

OK :) cool, merged. Thanks!