Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support quorum options #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dalecaru
Copy link
Contributor

@dalecaru dalecaru commented Jan 2, 2015

This adds support for quorum options in #save, #delete, #reload, ::[] and ::exists? methods.

It adds an options hash as a parameter to the methods mentioned above. The quorum should be set with the :quorum key.

Usage:

# Ork::Model#save
event.save(quorum: { w: 1, dw: 1 }) # same options than Riak::RObject#store method (:r, :w and :dw)

# Ork::Model#delete
event.delete(quorum: 2) # deletes with :rw option

# Ork::Model#reload
event.reload(quorum: 1) # reloads with :r option

# Ork::Model::[]
Event[some_id, quorum: 1] # reads with :r option

# Ork::Model::exists?
Event.exists?(some_id, quorum: 1) # reads with :r option

quorum = options.fetch(:quorum, {})

unless quorum.empty?
raise ArgumentError.new("invalid quorum option") unless quorum.keys.select { |key| !quorum_types.include?(key) }.empty?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [127/80]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good practice is to create a specific error like InvalidQuorumOption < ArgumentError
Also, if you have to map/select a collection, please use a different variable.

@emancu
Copy link
Owner

emancu commented Jan 2, 2015

Did you consider to create a Quorum class to handle all the methods around quorum?

@emancu
Copy link
Owner

emancu commented Jan 5, 2015

I'll do a review to this PR by the end of the week

@dalecaru
Copy link
Contributor Author

This is an extract of what we are using in a internal project. I agree with you in your comments, but before that, do you agree with the public API?. Is this what you had in mind? (Issue #1)

@emancu
Copy link
Owner

emancu commented Jan 26, 2015

@damiancaruso I am sorry this PR is taking so long.
I am very busy with toml-rb and i'm in the middle of vacations.

About the public API, I had two completely different ideas, one is covered in this PR, the other one is something like this:

Post.quorum(w: 4).save
Post.quorum(r: 2).reload

I'm not sure right now, I would like to brainstorm about both implementations and API.
And also, start thinking on support riak 2.

BTW I don't like to change the [] method to support quorum options. I want to keep that method simple and if you need the quorum option for read, you should use other method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants