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.

Sign up for GitHub

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

Jump to bottom

Warn in startup if dvv_enabled is false [JIRA: RCS-224] #1176

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Warn in startup if dvv_enabled is false [JIRA: RCS-224] #1176

wants to merge 2 commits into from

Conversation

kuenishi
Copy link
Contributor

@kuenishi kuenishi commented Jul 1, 2015

No description provided.

@shino
Copy link
Contributor

shino commented Jul 9, 2015

For completeness, one more assertion of lww=false is desirable, isn't it?

@Basho-JIRA Basho-JIRA changed the title Warn in startup if dvv_enabled is false Warn in startup if dvv_enabled is false [JIRA: RCS-224] Jul 10, 2015
@kuenishi kuenishi added this to the 2.1.0 milestone Jul 10, 2015
@kuenishi
Copy link
Contributor Author

add check on lww=false, too. Also, result of riakc_pb_socket:get_bucket_type(Pid, <<"default">>) does not include dvv_enabled explicitly, while HTTP API does. It may be another issue, but any thoughts?

@kuenishi
Copy link
Contributor Author

Ooops, dialyzer.

@kuenishi kuenishi force-pushed the feature/warn-dvv-enabled branch from b0b07dc to e67db50 Compare July 13, 2015 05:39
@kuenishi
Copy link
Contributor Author

Okay, pleased dialyzer

@shino
Copy link
Contributor

shino commented Jul 21, 2015

I added following lines to riak.conf and start riak and riak-cs, then got warning about dvv_enabled

[warning] dvv_enabled is strongly recommended to be true (currently undefined)

Configuration is wrong?

@kuenishi
Copy link
Contributor Author

I also tested with this config:

buckets.default.allow_mult = true
buckets.default.merge_strategy = 2

My test result: https://gist.github.com/kuenishi/eeb601889e985a6072fb

My conclusion: it's like a Riak bug in PB API.

@kuenishi
Copy link
Contributor Author

Found the culprit: https://github.com/basho/riak_pb/blob/develop/src/riak_pb_codec.erl#L241-L299

The decoder ignores dvv_enabled and write_once .

@shino
Copy link
Contributor

shino commented Jul 21, 2015

Then, this PR will be deferred to 2.1.1?

@kuenishi kuenishi mentioned this pull request Jul 21, 2015
Open
@kuenishi
Copy link
Contributor Author

Opened basho/riak_pb#127 as a right fix. We'll have to work on workaround, too...

@kuenishi
Copy link
Contributor Author

Then, this PR will be deferred to 2.1.1?

If fortunately that is included in Riak 2.1.2, I'd be happy but will push my workaround.

shino
shino reviewed Jul 22, 2015
View reviewed changes
src/riak_cs_app.erl
@@ -57,14 +55,11 @@ stop(_State) ->
sanity_check(true, {ok, true}) ->
riak_cs_sup:start_link();
sanity_check(false, _) ->
_ = lager:error("You must update your Riak CS app.config. Please see the"
_ = lager:error("You must update your Riak CS riak-cs.conf. Please see the"
"release notes for more information on updating you"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to add white spaces at the end of these two lines because concatenated string is shown in the log.

@kuenishi
Copy link
Contributor Author

Dropping this off of 2.1.0 because it's silly to connect to HTTP port just for checking dvv_enabled. Will revive this after basho/riak_pb#127 is merged.

@kuenishi kuenishi modified the milestones: 2.2.0, 2.1.0 Jul 28, 2015
@shino
Copy link
Contributor

shino commented Jan 19, 2016

This is on-hold because of riak kv side issue. This is worth to put in future version of riak_cs.

@kuenishi kuenishi removed their assignment Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
JIRA: To Do
Projects
None yet
Milestone
2.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@kuenishi @shino @Basho-JIRA