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

Refactor syncWithPrimary and replicaTryPartialResynchronization #1476

Open
wants to merge 9 commits into
base: unstable
Choose a base branch
from

Conversation

nitaicaro
Copy link
Contributor

In continuation to #945

syncWithPrimary:

  • Refactored returns and memory frees to single goto error/goto ok label
  • Refactored repl_state state machine from if-else format to switch-case format
  • Besides changing the repl_state, all state machine logic moved to helper functions

replicaTryPartialResynchronization:
This function was performing two different jobs based on the value of the read_reply argument -

  • read_reply == 0: Sends the PSYNC command to the primary server.
  • read_reply == 1: Reads and processes the reply to the PSYNC command.

This change simplifies the logic by clearly separating the writing and reading stages of the PSYNC process.

The PR was split into individual commits for easy review.

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

Attention: Patch coverage is 86.98630% with 19 lines in your changes missing coverage. Please review.

Project coverage is 70.85%. Comparing base (9f4503c) to head (a071443).
Report is 2 commits behind head on unstable.

Files with missing lines Patch % Lines
src/replication.c 86.98% 19 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1476      +/-   ##
============================================
- Coverage     70.86%   70.85%   -0.02%     
============================================
  Files           119      119              
  Lines         64859    64882      +23     
============================================
+ Hits          45963    45972       +9     
- Misses        18896    18910      +14     
Files with missing lines Coverage Δ
src/replication.c 87.60% <86.98%> (+0.07%) ⬆️

... and 11 files with indirect coverage changes

@naglera naglera requested a review from PingXie December 23, 2024 12:38
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.

This kind of refactoring is really valuable for the long-term health of Valkey. It makes the codebase easier to understand, maintain, and improve, which ultimately helps us deliver better features faster. Thanks @nitaicaro and @naglera!

My main focus in this review is on the high level code structure, which looks great to me. That said, the use of goto in syncWithPrimary stands out. Let's aim to eliminate all goto statements from that function. I'll take another look and provide more detailed feedback once that's addressed.

Comment on lines +2997 to +2998
/**
* Handles the initial step of the partial resynchronization process by
Copy link
Member

Choose a reason for hiding this comment

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

nit - I don't remember we start the comment block with /**?

Suggested change
/**
* Handles the initial step of the partial resynchronization process by
/* Handles the initial step of the partial resynchronization process by

Comment on lines +3008 to 3009
* step is to call replicaProcessPsyncReply().
*/
Copy link
Member

Choose a reason for hiding this comment

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

nit - I believe this is more common.

Suggested change
* step is to call replicaProcessPsyncReply().
*/
* step is to call replicaProcessPsyncReply(). */

Comment on lines +3063 to +3064


Copy link
Member

Choose a reason for hiding this comment

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

I personally like using double empty lines between functions but the convention is single empty link I think

@@ -3738,7 +3763,7 @@ void syncWithPrimary(connection *conn) {
server.repl_transfer_read = 0;
server.repl_transfer_last_fsync_off = 0;
server.repl_transfer_lastio = server.unixtime;
return;
goto ok;

no_response_error: /* Handle receiveSynchronousResponse() error when primary has no reply */
Copy link
Member

Choose a reason for hiding this comment

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

These goto labels (no_response_error, error, write_error, and ok) have really gone too far now. Let's consolidate them into a single syncWithPrimaryHandleError function. I am hopeful that we can remove all the gotos in this function.

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.

3 participants