-
Notifications
You must be signed in to change notification settings - Fork 817
Add multi-database support to cluster mode #1671
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1671 +/- ##
============================================
- Coverage 71.02% 70.84% -0.18%
============================================
Files 123 123
Lines 66116 66173 +57
============================================
- Hits 46956 46879 -77
- Misses 19160 19294 +134
🚀 New features to boost your workflow:
|
src/db.c
Outdated
@@ -1728,12 +1714,6 @@ void swapMainDbWithTempDb(serverDb *tempDb) { | |||
void swapdbCommand(client *c) { | |||
int id1, id2; | |||
|
|||
/* Not allowed in cluster mode: we have just DB 0 there. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that be enough for swapdb to work in cluster mode? What will happen in setup with 2 shards, each responsible for half of slots in db's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this implementation SWAPDB must be executed in all primary nodes. There are three options:
- Allow SWAPDB and shift responsibility to the user – Risky, non-atomic, can cause temporary inconsistency and data corruption. Needs strong warnings.
- Keep SWAPDB disabled in cluster mode – Safest, avoids inconsistency.
- Make SWAPDB cluster-wide and atomic or – Complex, unclear feasibility.
I think option 2 is the safest bet. @JoBeR007 wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is SWAPDB replicated as a single command? Then it's atomic.
If it's risky, it's risky in standslone mode with replicas too, right?
I think we can allow it. Swapping the data can only be done in some non-realtime workloads anyway I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think risky because of replication and risky because of the need to execute SWAPDB on all primary nodes are unrelated just because as a user you can't control first, but user is the main risk in the second case.
I would keep SWAPDB disabled in cluster mode, if we decide to continue with this implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In cluster mode, consistency is per slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, FLUSHDB is very similar in this regard. If a failover happens just before this command has been propagated to replicas, it's a big thing, but it's no surprise I think. The client can use WAIT or check replication offset to make sure the FLUSHDB or SWAPDB was successful on the replicas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding this, I think it is not just an issue of Multi-database but is more related to atomic slot migration. If a shard is in a stable state (not undergoing slot migration), then flushdb
/flushall
/swapdb
are safe. However, if slot migration is in progress, it might lead to data inconsistency.
I think this needs to be considered alongside atomic-slot-migration:
- During the ATM process, for slots being migrated, if we encounter
flushall
/flushdb
, we can send a command likeflushslot
orflushslotall
to the target shard - As for
swapdb
, I recommend temporarily prohibiting execution during the ATM process
@PingXie @enjoy-binbin , please also take note of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense. @murphyjacob4 FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a comment on the issue about this, but also worth mentioning it's hard to orchestrate SWAPDB
. Even in steady state, flushdb
and flushall
are idempotent (you can send them multiple times) but swapdb isn't. If a command times out on one node, it's hard to reason about if it was successful and how to retry it. I think we should continue to disable SWAPDB
in cluster mode for now, unless we introduce an idempotent way to do the swap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe introduce UUID tracking for SWAPDB requests works.
disabling SWAPDB for now.
I'm happy that we did "Unified db rehash method for both standalone and cluster #12848" when developing kvstore , which made the implementation of multi-database simpler. |
There was a problem hiding this 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 history to SWAPDB
, SELECT
, MOVE
json files to indicate it's supported since 9.0.
src/db.c
Outdated
@@ -1728,12 +1714,6 @@ void swapMainDbWithTempDb(serverDb *tempDb) { | |||
void swapdbCommand(client *c) { | |||
int id1, id2; | |||
|
|||
/* Not allowed in cluster mode: we have just DB 0 there. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a comment on the issue about this, but also worth mentioning it's hard to orchestrate SWAPDB
. Even in steady state, flushdb
and flushall
are idempotent (you can send them multiple times) but swapdb isn't. If a command times out on one node, it's hard to reason about if it was successful and how to retry it. I think we should continue to disable SWAPDB
in cluster mode for now, unless we introduce an idempotent way to do the swap.
3dc03d6
to
af2d46a
Compare
documentation: valkey-io/valkey-doc#242 |
It looks like there are still some test cases failed related to multiply db feature. Please fix them first, Thanks |
538e23e
to
63151ae
Compare
Signed-off-by: xbasel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with the current iteration.
Co-authored-by: Ran Shidlansik <[email protected]> Signed-off-by: xbasel <[email protected]>
Co-authored-by: Ran Shidlansik <[email protected]> Signed-off-by: xbasel <[email protected]>
Signed-off-by: xbasel <[email protected]>
Signed-off-by: xbasel <[email protected]>
I created issues for everything I noticed was a followup, wanted to merge to allow other PRs like atomic slot migration to rebase. |
Almost all tests in Daily failed today. What happened here?
https://github.com/valkey-io/valkey/actions/runs/14826557992 |
Re-adds a statement to restore the `singledb` config that was accidentally removed in PR valkey-io#1671. Signed-off-by: xbasel <[email protected]>
Re-adds a statement to restore the `singledb` config that was accidentally removed in PR valkey-io#1671. Signed-off-by: xbasel <[email protected]>
Re-adds a statement to restore the `singledb` config that was accidentally removed in PR #1671. Fixes #2049 Signed-off-by: xbasel <[email protected]>
One of the new tests that was added uses `CONFIG GET PORT`, which isn't right one for TLS. Also removed some other use of the helper which aren't actually used. Introduced as part of #1671. --------- Signed-off-by: Madelyn Olson <[email protected]>
## cluster: add multi-database support in cluster mode Add multi-database support in cluster mode to align with standalone mode and facilitate migration. Previously, cluster mode was restricted to a single database (DB0). This change allows multiple databases while preserving the existing slot-based key distribution. ### Key Features: - Database-Agnostic Hashing. The hashing algorithm is unchanged. Identical keys always map to the same slot across all databases, ensuring consistent key distribution and compatibility with existing single-database setups. - Multi-DB commands support. SELECT, MOVE, and COPY are now supported in cluster mode. - Fully backward compatible with no API changes. - SWAPDB is not supported in cluster mode. It is unsafe due to inconsistency risks. ### Command-Level Changes: - SELECT / MOVE / COPY are now supported in cluster mode. - MOVE / COPY (with db) are rejected (TRYAGAIN error) during slot migration to prevent multi-DB inconsistencies. - SWAPDB will return an error if used when cluster mode is enabled. - GETKEYSINSLOT, COUNTKEYSINSLOT and MIGRATE will operate in the context of the selected database. This means, for example, that migrating keys in a slot will require iterating and repeating across all databases. ### Slot Migration Process: - Multi-DB support in cluster mode affects slot migration. Operators should now iterate over all the configured databases. ### Transaction Handling (MULTI/EXEC): - getNodeByQuery key lookup behavior changed: - No key lookups when queuing commands in MULTI, only cross-slot validation. - Key lookups happen at EXEC time. - SELECT inside MULTI/EXEC is now checked, ensuring key validation uses the selected DB at lookup. ### Valkey-cli: - valkey-cli has been updated to support resharding across all databases. ### Configuration: - Introduce new configuration `cluster-databases`. The new configuration controls the maximal number of databases in cluster mode. Implements valkey-io#1319 --------- Signed-off-by: xbasel <[email protected]> Signed-off-by: zhaozhao.zz <[email protected]> Co-authored-by: zhaozhao.zz <[email protected]> Co-authored-by: Viktor Söderqvist <[email protected]> Co-authored-by: Madelyn Olson <[email protected]> Co-authored-by: Ran Shidlansik <[email protected]>
Re-adds a statement to restore the `singledb` config that was accidentally removed in PR valkey-io#1671. Fixes valkey-io#2049 Signed-off-by: xbasel <[email protected]>
One of the new tests that was added uses `CONFIG GET PORT`, which isn't right one for TLS. Also removed some other use of the helper which aren't actually used. Introduced as part of valkey-io#1671. --------- Signed-off-by: Madelyn Olson <[email protected]>
cluster: add multi-database support in cluster mode
Add multi-database support in cluster mode to align with standalone mode
and facilitate migration. Previously, cluster mode was restricted to a
single database (DB0). This change allows multiple databases while
preserving the existing slot-based key distribution.
Key Features:
Identical keys always map to the same slot across all databases,
ensuring consistent key distribution and compatibility with
existing single-database setups.
cluster mode.
Command-Level Changes:
This means, for example, that migrating keys in a slot will require iterating and repeating across all databases.
Slot Migration Process:
Transaction Handling (MULTI/EXEC):
validation.
uses the selected DB at lookup.
Valkey-cli:
Configuration:
cluster-databases
.The new configuration controls the maximal number of databases in cluster mode.
Implements #1319