basho / riak_core

Distributed systems infrastructure used by Riak.
Apache License 2.0
1.23k stars 392 forks source link

Remove 'vnode_vclocks' default setting in riak.conf #448

Closed dmitrizagidulin closed 10 years ago

dmitrizagidulin commented 10 years ago

To simplify the default riak.conf file, would it make sense to remove the following section?

## Switch to vnode-based vclocks rather than client ids.  This
## significantly reduces the number of vclock entries.
## Only set on if *all* nodes in the cluster are upgraded to 1.0
vnode_vclocks = on

Given that: 1) It's on by default anyway 2) The comments refer to "once you upgrade to 1.0" and this is 2.0 3) That this is the new-style riak.conf, and if somebody is migrating from pre-1.0, they're probably keeping their app.config 4) The new CRDTs require vnode_clocks to be on in order to work

it would make sense to just leave it out entirely from a newly-generated riak.conf, and have it be a custom setting that people can add if absolutely necessary (either to riak.conf or the advanced config).

russelldb commented 10 years ago

YES! YES! YES! And, while we're at it, let's remove clientid vclocks from riak for 2.0!

dmitrizagidulin commented 10 years ago

Also on the subject of upgrade to 1.0, deprecate this section:

## mapred_2i_pipe indicates whether secondary-index
## MapReduce inputs are queued in parallel via their own
## pipe ('true'), or serially via a helper process
## ('false' or undefined).  Set to 'false' or leave
## undefined during a rolling upgrade from 1.0.
mapred_2i_pipe = on
evanmcc commented 10 years ago

are dots or something like them going into 2.0 for absolute certain? have we reached out to the customer that we know is still using vnode_vclocks </broken record>

On Thu, Nov 7, 2013 at 9:31 AM, Dmitri Zagidulin notifications@github.comwrote:

Also on the subject of upgrade to 1.0, deprecate this section:

mapred_2i_pipe indicates whether secondary-index

MapReduce inputs are queued in parallel via their own

pipe ('true'), or serially via a helper process

('false' or undefined). Set to 'false' or leave

undefined during a rolling upgrade from 1.0.

mapred_2i_pipe = on

— Reply to this email directly or view it on GitHubhttps://github.com/basho/riak_core/issues/448#issuecomment-27986984 .

dmitrizagidulin commented 10 years ago

@evanmcc Just to clarify -- I'm only advocating for removing the 'vnode_clocks' section from the default riak.conf (freshly downloaded/compiled) Riak 2.0 installations. If customers are still using the setting 'vnode_clocks=off' (meaning, they're using pre 1.0 client-id-based vclocks vs the vnode-based ones) and are upgrading to 2.0, they'll be keeping their legacy app.config files (which will override the riak.conf anyway).

russelldb commented 10 years ago

@evanmcc nothing is certain, but I'm working on it. No we haven't reached out, do we even know who "they" are? I'm happy to talk to anyone still using clientId vclocks, 'cos a guarantee they're doing it wrong.

macintux commented 10 years ago

@russelldb In case this is still an outstanding question, last I heard we had a customer using them for performance reasons. Check with the CSE team

evanmcc commented 10 years ago

The customer in question has been aquihired, and their service is going to be shut down, so it's less of an issue, I think. On Jan 5, 2014 4:37 PM, "John Daily" notifications@github.com wrote:

@russelldb https://github.com/russelldb In case this is still an outstanding question, last I heard we had a customer using them for performance reasons. Check with the CSE team

— Reply to this email directly or view it on GitHubhttps://github.com/basho/riak_core/issues/448#issuecomment-31620774 .

jrwest commented 10 years ago

this seems like something we should decide for 2.0-RC. @joedevivo @seancribbs @jonmeredith @gburd @michellep ?. I've marked it as such for now.

gburd commented 10 years ago

I'd like to see both of those confusing and out-dated sections disappear from the riak.conf file.

jrwest commented 10 years ago

ok, sounds like it should stay on 2.0-RC then. thanks @gburd

engelsanchez commented 10 years ago

I don't see this setting in the cuttlefish schemas. I guess it has been removed already. The only mention I found is in a test schema: cuttlefish/test/riak.schema. @joedevivo do you know if that is correct and we can close this issue?

joedevivo commented 10 years ago

removed at some point. vnode_vclocks is now false unless overridden in advanced.config

joedevivo commented 10 years ago

For the record, cuttlefish/test/riak.schema was the first schema we did, and is pretty close to 1.4.

engelsanchez commented 10 years ago

Thanks @joedevivo. I'm closing this issue then.

seancribbs commented 10 years ago

Umm, vnode_vclocks should be true by default (is it that way in the code?). What happened?

joedevivo commented 10 years ago

You're right @seancribbs, I misread the source, thinking that the rpc:call was returning the default false on line 126, but really, the default is coming from the capabilities system.

/Users/joe/dev/basho/riak_ee/deps/riak_kv/src/riak.erl:
  121: vnode_vclocks(Node) ->
  122      case rpc:call(Node, riak_core_capability, get,
  123:                   [{riak_kv, vnode_vclocks}]) of
  124          {badrpc, {'EXIT', {undef, _}}} ->
  125              rpc:call(Node, app_helper, get_env,
  126:                      [riak_kv, vnode_vclocks, false]);
  127          Result ->
  128              Result
seancribbs commented 10 years ago

@joedevivo :+1: cool