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

Fix create db options on secondary shard creation #2690

Merged
merged 3 commits into from Mar 27, 2020

Conversation

chewbranca
Copy link
Contributor

Overview

This is a continuation of the work in #2672 to fix issues around secondary shard creation when nodes re-enter a cluster after a database was created while the node was down (or potentially in maintenance mode).

This PR introduces logic to handle the following four scenarios while opening a db in context of fabric_rpc and mem3_rpc:

  1. the db already exists
    - just return the db as normal
  2. the db does not exist locally, but the dbs db doc with db create options is present
    - load the db create options and create the local shard
  3. the db does not exist locally but it's not a sharded database
    - just create the local db as expected
  4. the db does not exist locally, it's a sharded database, and the dbs db doc is not present
    - throw an error and prevent creating the db with incomplete settings

This PR changes the behavior of scenario 2) by loading the appropriate config and creating the db with the desired parameters, and of scenario 4) by no longer allowing the db to be created inappropriately.

For scenario three, I've taken the approach of not loading config as a non sharded db won't have a dbs db doc. In particular here, we want to properly handle updates to internal dbs like _dbs and also sys_dbs like _users.

Another open item here is the merging of the db create options and the provided options. Right now, I'm just doing DbOpts ++ Opts basically, but I wrote a functions opts_merge/{2,3} that will merge two proplists, prioritizing the first list of items, and supports atoms. It's... a bit gnarly, and I'm not convinced we should use it. So we need to figure out what to do with the options lists here, but I think we might be ok just combining the two.

While in the general neighborhood, I took the opportunity to de-duplicate some of the shards_db config lookups as well. I've isolated those changes to a dedicated commit.

Testing recommendations

This needs more testing. I've got a WIP commit for adding a test suite here that is not yet complete. I figured it was worthwhile to get this PR out for feedback while continuing to work on the tests.

Related Issues or Pull Requests

#2672

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

@chewbranca
Copy link
Contributor Author

BTW... I believe this issue also extends to PSE databases created while a node was out of rotation. Basically anything that relies on database creation config options, which is PSE and PQ (anything else?).

Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

We need to add tests for options actually being applied as well.

src/fabric/test/eunit/fabric_rpc_tests.erl Show resolved Hide resolved
src/fabric/test/eunit/fabric_rpc_tests.erl Show resolved Hide resolved
src/mem3/src/mem3_util.erl Outdated Show resolved Hide resolved
src/mem3/src/mem3_util.erl Outdated Show resolved Hide resolved
Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

Super important change here.

src/fabric/test/eunit/fabric_rpc_tests.erl Outdated Show resolved Hide resolved
Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

+1

@wohali
Copy link
Member

wohali commented Mar 26, 2020

Shouldn't this and #2672 target the 3.x branch, from which our next release will be cut?

@rnewson
Copy link
Member

rnewson commented Mar 26, 2020

yes. :)

Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

+1

@chewbranca chewbranca merged commit 9ee8244 into master Mar 27, 2020
@davisp davisp deleted the fix-create-db-options branch March 27, 2020 19:54
rnewson pushed a commit that referenced this pull request Apr 1, 2020
Fix create db options on secondary shard creation
rnewson pushed a commit that referenced this pull request Apr 2, 2020
Fix create db options on secondary shard creation
rnewson pushed a commit that referenced this pull request Apr 2, 2020
Fix create db options on secondary shard creation
rnewson pushed a commit that referenced this pull request Apr 2, 2020
Fix create db options on secondary shard creation
dottorblaster pushed a commit to dottorblaster/couchdb that referenced this pull request Apr 2, 2020
Fix create db options on secondary shard creation
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.

None yet

4 participants