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

Commit Blocks in Snapshot and Enhance UT #258

Merged
merged 2 commits into from
Feb 19, 2025
Merged

Commit Blocks in Snapshot and Enhance UT #258

merged 2 commits into from
Feb 19, 2025

Conversation

yuwmao
Copy link
Contributor

@yuwmao yuwmao commented Feb 18, 2025

Commit Blocks in snapshot, or the blocks will be overwritten after restart.
Add more UTs to test logic when follower/leader restart.

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 15.38462% with 11 lines in your changes missing coverage. Please review.

Project coverage is 62.46%. Comparing base (1746bcc) to head (d177b4c).
Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
...ib/homestore_backend/replication_state_machine.cpp 0.00% 7 Missing ⚠️
...lib/homestore_backend/snapshot_receive_handler.cpp 20.00% 3 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #258      +/-   ##
==========================================
- Coverage   63.15%   62.46%   -0.70%     
==========================================
  Files          32       33       +1     
  Lines        1900     2587     +687     
  Branches      204      307     +103     
==========================================
+ Hits         1200     1616     +416     
- Misses        600      808     +208     
- Partials      100      163      +63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yuwmao yuwmao force-pushed the ut branch 2 times, most recently from 3c2ab19 to 851cb39 Compare February 18, 2025 09:17
Copy link
Collaborator

@xiaoxichen xiaoxichen left a comment

Choose a reason for hiding this comment

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

lgtm, a bit concern on the leader we assigned during creation might not be the leader when we doing the test (if leader switch happened in between). But as far as this is testing code, it 's LGTM

@yuwmao
Copy link
Contributor Author

yuwmao commented Feb 19, 2025

lgtm, a bit concern on the leader we assigned during creation might not be the leader when we doing the test (if leader switch happened in between). But as far as this is testing code, it 's LGTM

Yes, it could occur, will improve it by using get_leader_id instead of constant initial_leader_replica_id as far as possible to reduce the chances of this case occurring.

@xiaoxichen
Copy link
Collaborator

lgtm, a bit concern on the leader we assigned during creation might not be the leader when we doing the test (if leader switch happened in between). But as far as this is testing code, it 's LGTM

Yes, it could occur, will improve it by using get_leader_id instead of constant initial_leader_replica_id as far as possible to reduce the chances of this case occurring.

But if the leader is replica-0 it is hard to handle :). Maybe explicity request leadership is better . Anyway lets improve later, feel free to merge as is

@yuwmao
Copy link
Contributor Author

yuwmao commented Feb 19, 2025

lgtm, a bit concern on the leader we assigned during creation might not be the leader when we doing the test (if leader switch happened in between). But as far as this is testing code, it 's LGTM

Yes, it could occur, will improve it by using get_leader_id instead of constant initial_leader_replica_id as far as possible to reduce the chances of this case occurring.

But if the leader is replica-0 it is hard to handle :). Maybe explicity request leadership is better . Anyway lets improve later, feel free to merge as is

Ok, if it supports to force change leadership to the specified member, that will be easy, we can request and wait for leader.

@yuwmao yuwmao merged commit 13b5911 into eBay:main Feb 19, 2025
26 checks passed
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