Refactor syncWithPrimary and replicaTryPartialResynchronization#1476
Refactor syncWithPrimary and replicaTryPartialResynchronization#1476PingXie merged 1 commit intovalkey-io:unstablefrom
Conversation
nitaicaro
force-pushed
the
unstable
branch
2 times, most recently
from
e52808f to
6df1d3c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1476 +/- ##
============================================
+ Coverage 71.03% 71.04% +0.01%
============================================
Files 123 123
Lines 66033 66087 +54
============================================
+ Hits 46908 46954 +46
- Misses 19125 19133 +8
🚀 New features to boost your workflow:
|
PingXie
left a comment
PingXie
left a comment
There was a problem hiding this comment.
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.
Done |
nitaicaro
force-pushed
the
unstable
branch
2 times, most recently
from
2e42f15 to
813ef82
Compare
|
@nitaicaro thanks for the reminder! I didn't forget about this one :) I am planning to continue my review next week. |
|
btw, there seems to be some merge conflicts. do you have a plan to resolve them soon? |
PingXie
left a comment
PingXie
left a comment
There was a problem hiding this comment.
Thanks for your patience, @nitaicaro. The overall code structure LGTM and it is a lot cleaner!
I have some local comments that I think should be easy to address. We can aim at merging this PR shortly after 8.1 GA.
PingXie
left a comment
PingXie
left a comment
There was a problem hiding this comment.
LGTM with only one last comment. Will approve once it is addressed. Thanks @nitaicaro
PingXie
left a comment
PingXie
left a comment
There was a problem hiding this comment.
This looks great! Thanks so much for your patience, @nitaicaro! Really appreciate it. Let's merge it!
nitaicaro
force-pushed
the
unstable
branch
6 times, most recently
from
924274b to
d3ef66f
Compare
nitaicaro
force-pushed
the
unstable
branch
3 times, most recently
from
9ad7e78 to
ccdb1c1
Compare
Signed-off-by: Nitai Caro <[email protected]>
|
@valkey-io/core-team anyone else would like to take a look at this pr before it gets merged? |
enjoy-binbin
left a comment
enjoy-binbin
left a comment
There was a problem hiding this comment.
I am worry this will cause conflict with #1949. @murphyjacob4
|
Sadly, a merge conflict seems inevitable. I wouldn't block this PR on #1949, I can handle the merge conflicts in that PR |
…ey-io#1476) In continuation to valkey-io#945 **syncWithPrimary:** - Refactored all error handling to function `syncWithPrimaryHandleError` - 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. Signed-off-by: Nitai Caro <[email protected]> Co-authored-by: Nitai Caro <[email protected]>
5 participants
In continuation to #945
syncWithPrimary:
syncWithPrimaryHandleErrorreplicaTryPartialResynchronization:
This function was performing two different jobs based on the value of the read_reply argument -
This change simplifies the logic by clearly separating the writing and reading stages of the PSYNC process.