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

Add bucket property node_confirms for physical diversity #1663

Merged
merged 8 commits into from
Mar 22, 2018
Merged

Add bucket property node_confirms for physical diversity #1663

merged 8 commits into from
Mar 22, 2018

Conversation

russelldb
Copy link
Member

@russelldb russelldb commented Feb 22, 2018

This is an upstream PR of a few related, merged, PRs/branches from the nhs-riak fork.

The original PRs are:

See doc/Node-Diversity.md for full details.

Briefly, some Riak deployments require that a write is on multiple
physical nodes before they're safe.

Given a good ring, using Primaries is one way to ensure this physical
diversity is to use pw=2 (for example) to ensure two physical nodes
for the write. This has an availability cost. With 5 nodes and 2 nodes
down storing on 2 physical nodes is still very possible, but 2
primaries will leave a range of preflists unavailable to write. This
commit adds a bucket property and put option of node_confirms to
declare the number of diverse physical node acks required for a write
to be successful. For the above example node_confirms=2 would be
used, and as long as any two diverse nodes (primary OR fallback)
reply, the write is a success.

Further work fixed a related bug (see the commit message for b23b070 for details.

There are associated PB, riakc, httpc, riak-test, and riak_core PRs:

ranisen and others added 8 commits January 9, 2018 16:33
Briefly, some Riak deployments require that a write is on multiple
physical nodes before they're safe.

Given a good ring, using Primaries is one way to ensure this physical
diversity is to use `pw=2` (for example) to ensure two physical nodes
for the write. This has an availability cost. With 5 nodes and 2 nodes
down storing on 2 physical nodes is still very possible, but 2
primaries will leave a range of preflists unavailable to write. This
commit adds a bucket property and put option of `node_confrims` to the
declare the number of diverse physical node acks required for a write
to be successful. For the above example `node_confirms=2` would be
used, and as long as any two diverse nodes (primary OR fallback)
reply, the write is a success.
Description and justification for new node_confirms option.
When adding `node_confirms` a node confirms failure threshold was also
added, this brought the arity to 11 for put core init. All quorum
failure thresholds are calculated as `N+Q-1`. This commit moves that
calculation inside the put core. The arity of init is still 8, which is
high.
Take it or leave it, but it's easier to edit without mega length lines,
imo.
Lessens the probability that the links will become broken.
objects/counters/datatypes all
Typed buckets do not automatically inherit newly added default
properties (old-skool default buckets do thanks to the existence of
bucket fixups.)

As per the bug report, when PUTing to a Typed bucket after upgrade to
2.2.5 the call to riak_kv_util:expand_rw_value/4 throws if the
property `node_confirms` is not in the bucket properties, and for a
typed bucket created and activated before upgrade it won't be.

This is somewhat contentious. Ex-Basho @seancribbs has pointed out
that there is a reason for that throw, and that all nodes should agree
on the value, and misconfiguration can be an issue here.  There are
perhpas more complexities (we're retroactivley adding a reserved word,
for example.) Caveats in docs and probability should guard against
that latter though.

Given that the reasons not to do this are lost to time, and the
alternatives unpleasant, I'm content that this is a reasonable way for
typed buckets to inherit **NEW** properties added as defaults after
typed bucket creation.

There is an associated riak_core commit.
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Would be nice if riak_kv_pb_counter.erl could die in fire - but outside the scope of this p/r - everything else looks sensible.

@ghost
Copy link

ghost commented Feb 26, 2018

+1

@russelldb
Copy link
Member Author

@bryanhuntesl re vsn 1.4 counter, it will be gone in 3.0

@martincox
Copy link
Contributor

Neat fix on the pre-upgrade bucket-types. 👍 +1

@russelldb russelldb merged commit b4e9ba1 into basho:develop-2.2 Mar 22, 2018
@russelldb russelldb deleted the rdb/upstream-pr/node-confirms branch March 22, 2018 15:52
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