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

Replicate slot migration states via RDB aux fields #586

Merged
merged 10 commits into from
Jun 8, 2024

Conversation

PingXie
Copy link
Member

@PingXie PingXie commented Jun 2, 2024

Replicate slot migration states via RDB aux fields to eliminate the brief window of inconsistency that could break the high availability guarantee intended by #445. Previously, these states were replicated using explicit CLUSTER SETSLOT commands after full resync is completed, which was not only a hack but also created this window where a new replica can become online without knowing its primary's slot migration states.

Additionally, this PR introduces a generic framework for extending RDB aux fields without strong coupling. Now other subsystems in the server, such as cluster bus, can inject aux fields into RDB without rdb.c taking a code dependency on them via hardcoded functions.

Fix #419

@PingXie PingXie requested a review from madolson June 2, 2024 07:18
Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@google.com>
@PingXie PingXie changed the title Replicate slot migrating states via RDB aux fields Replicate slot migration states via RDB aux fields Jun 2, 2024
Copy link

codecov bot commented Jun 2, 2024

Codecov Report

Attention: Patch coverage is 91.37931% with 5 lines in your changes missing coverage. Please review.

Project coverage is 70.18%. Comparing base (54c9747) to head (19d3707).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #586      +/-   ##
============================================
- Coverage     70.20%   70.18%   -0.02%     
============================================
  Files           110      110              
  Lines         60007    60042      +35     
============================================
+ Hits          42130    42143      +13     
- Misses        17877    17899      +22     
Files Coverage Δ
src/latency.c 80.15% <ø> (ø)
src/replication.c 87.10% <ø> (-0.07%) ⬇️
src/server.c 88.92% <ø> (-0.02%) ⬇️
src/server.h 100.00% <ø> (ø)
src/cluster_legacy.c 86.58% <93.75%> (+0.14%) ⬆️
src/rdb.c 75.55% <88.46%> (+0.14%) ⬆️

... and 10 files with indirect coverage changes

Signed-off-by: Ping Xie <pingxie@google.com>
@PingXie PingXie requested a review from zuiderkwast June 2, 2024 08:28
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, just some minor comments.

src/cluster_legacy.c Outdated Show resolved Hide resolved
src/server.h Outdated Show resolved Hide resolved
src/rdb.c Outdated Show resolved Hide resolved
src/cluster_legacy.c Outdated Show resolved Hide resolved
Signed-off-by: Ping Xie <pingxie@google.com>
src/rdb.c Show resolved Hide resolved
PingXie and others added 5 commits June 3, 2024 13:52
Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@google.com>
@PingXie PingXie merged commit aad6769 into valkey-io:unstable Jun 8, 2024
18 checks passed
advaMosh pushed a commit to advaMosh/valkey that referenced this pull request Jun 10, 2024
Signed-off-by: Adva Moshkovitz <advamo7@gmail.com>
naglera pushed a commit to naglera/placeholderkv that referenced this pull request Jun 10, 2024
@zuiderkwast
Copy link
Contributor

Aux fields are ignored when the RDB is loaded by an older version, right?

We still need to do #421 for online replicas though, don't we?

@PingXie PingXie deleted the repl-slot-states-in-rdb branch June 13, 2024 18:22
enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request Jun 17, 2024
When we goto eoferr, we need to release the auxkey and auxval,
this is a cleanup, also explicitly check that decoder return
value is C_ERR.

Introduced in valkey-io#586.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin
Copy link
Member

enjoy-binbin commented Jun 17, 2024

Aux fields are ignored when the RDB is loaded by an older version, right?

yes

We still need to do #421 for online replicas though, don't we?

i guess the answer is also yes.

PingXie pushed a commit that referenced this pull request Jun 18, 2024
When we goto eoferr, we need to release the auxkey and auxval,
this is a cleanup, also explicitly check that decoder return
value is C_ERR.

Introduced in #586.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[NEW] Inject slot migration states into RDB
4 participants