Skip to content

Avoid to re-establish replication if node is already myself primary in CLUSTER REPLICATE#884

Merged
enjoy-binbin merged 3 commits intovalkey-io:unstablefrom
enjoy-binbin:fast_path_cluster_replicate
Aug 22, 2024
Merged

Avoid to re-establish replication if node is already myself primary in CLUSTER REPLICATE#884
enjoy-binbin merged 3 commits intovalkey-io:unstablefrom
enjoy-binbin:fast_path_cluster_replicate

Conversation

@enjoy-binbin
Copy link
Member

@enjoy-binbin enjoy-binbin commented Aug 11, 2024

If n is already myself primary, there is no need to re-establish the
replication connection.

In the past we allow a replica node to reconnect with its primary via
this CLUSTER REPLICATE command, it will use psync. But since #885, we
will assume that a full sync is needed in this case, so if we don't do
this, the replica will always use full sync.

…n CLUSTER REPLICATE

If n is already myself primary, there is no need to re-establish the
replication connection.

Signed-off-by: Binbin <[email protected]>
@codecov
Copy link

codecov bot commented Aug 11, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.57%. Comparing base (39f8bcb) to head (eb3ef43).
⚠️ Report is 821 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #884      +/-   ##
============================================
- Coverage     70.62%   70.57%   -0.05%     
============================================
  Files           112      112              
  Lines         61509    61512       +3     
============================================
- Hits          43440    43412      -28     
- Misses        18069    18100      +31     
Files with missing lines Coverage Δ
src/cluster_legacy.c 85.62% <100.00%> (+0.33%) ⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

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

I can't think of anything that could go wrong but it is indeed a behavior change.

@valkey-io/core-team FYI

@PingXie PingXie added the release-notes This issue should get a line item in the release notes label Aug 12, 2024
Co-authored-by: Ping Xie <[email protected]>
Signed-off-by: Binbin <[email protected]>
@enjoy-binbin
Copy link
Member Author

I am merging this since #885 is merged, see top comment for more details

@enjoy-binbin enjoy-binbin merged commit 08aaeea into valkey-io:unstable Aug 22, 2024
@enjoy-binbin enjoy-binbin deleted the fast_path_cluster_replicate branch August 22, 2024 03:00
madolson pushed a commit that referenced this pull request Sep 2, 2024
…n CLUSTER REPLICATE (#884)

If n is already myself primary, there is no need to re-establish the
replication connection.

In the past we allow a replica node to reconnect with its primary via
this CLUSTER REPLICATE command, it will use psync. But since #885, we
will assume that a full sync is needed in this case, so if we don't do
this, the replica will always use full sync.

Signed-off-by: Binbin <[email protected]>
Co-authored-by: Ping Xie <[email protected]>
madolson pushed a commit that referenced this pull request Sep 3, 2024
…n CLUSTER REPLICATE (#884)

If n is already myself primary, there is no need to re-establish the
replication connection.

In the past we allow a replica node to reconnect with its primary via
this CLUSTER REPLICATE command, it will use psync. But since #885, we
will assume that a full sync is needed in this case, so if we don't do
this, the replica will always use full sync.

Signed-off-by: Binbin <[email protected]>
Co-authored-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
…n CLUSTER REPLICATE (valkey-io#884)

If n is already myself primary, there is no need to re-establish the
replication connection.

In the past we allow a replica node to reconnect with its primary via
this CLUSTER REPLICATE command, it will use psync. But since valkey-io#885, we
will assume that a full sync is needed in this case, so if we don't do
this, the replica will always use full sync.

Signed-off-by: Binbin <[email protected]>
Co-authored-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
…n CLUSTER REPLICATE (valkey-io#884)

If n is already myself primary, there is no need to re-establish the
replication connection.

In the past we allow a replica node to reconnect with its primary via
this CLUSTER REPLICATE command, it will use psync. But since valkey-io#885, we
will assume that a full sync is needed in this case, so if we don't do
this, the replica will always use full sync.

Signed-off-by: Binbin <[email protected]>
Co-authored-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
…n CLUSTER REPLICATE (valkey-io#884)

If n is already myself primary, there is no need to re-establish the
replication connection.

In the past we allow a replica node to reconnect with its primary via
this CLUSTER REPLICATE command, it will use psync. But since valkey-io#885, we
will assume that a full sync is needed in this case, so if we don't do
this, the replica will always use full sync.

Signed-off-by: Binbin <[email protected]>
Co-authored-by: Ping Xie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cluster release-notes This issue should get a line item in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants