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

Implement ReplicationStateMachine::write_snapshot_data #227

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

koujl
Copy link

@koujl koujl commented Nov 15, 2024

  • Implement ReplicationStateMachine::write_snapshot_data() and SnapshotReceiveHandler logic
  • Extract local_create_pg, local_create_shard and local_add_blob_info for follower data creation

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2024

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

Codecov Report

Attention: Patch coverage is 20.40816% with 195 lines in your changes missing coverage. Please review.

Please upload report for BASE (baseline_resync@7b9819d). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...lib/homestore_backend/snapshot_receive_handler.cpp 0.00% 133 Missing ⚠️
...ib/homestore_backend/replication_state_machine.cpp 0.00% 53 Missing ⚠️
src/lib/homestore_backend/hs_blob_manager.cpp 76.92% 4 Missing and 2 partials ⚠️
src/lib/homestore_backend/hs_pg_manager.cpp 87.50% 1 Missing and 1 partial ⚠️
src/lib/homestore_backend/hs_shard_manager.cpp 94.11% 1 Missing ⚠️

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

Additional details and impacted files
@@                Coverage Diff                 @@
##             baseline_resync     #227   +/-   ##
==================================================
  Coverage                   ?   51.53%           
==================================================
  Files                      ?       33           
  Lines                      ?     2053           
  Branches                   ?      236           
==================================================
  Hits                       ?     1058           
  Misses                     ?      906           
  Partials                   ?       89           

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

@koujl koujl marked this pull request as ready for review November 18, 2024 03:01

const int64_t snp_lsn_{0};
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see this is being used expect for the check to reuse this handler instance. Can we remove this field and the check and always reuse this handler instance?

Copy link
Author

Choose a reason for hiding this comment

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

The follower may receive a new snapshot transmission request if the leader crashes or if the previous snapshot transmission times out. The current LSN is necessary for the follower to recognize such cases and discard the current handler instance.

@koujl
Copy link
Author

koujl commented Nov 22, 2024

Added several TODOs as reminders for suggestions mentioned in the comments.

Further optimization & refactor will be posted within next PR.

@koujl koujl mentioned this pull request Nov 22, 2024
JacksonYao287
JacksonYao287 previously approved these changes Nov 25, 2024
Copy link
Collaborator

@JacksonYao287 JacksonYao287 left a comment

Choose a reason for hiding this comment

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

LGTM

@koujl koujl merged commit 303a0a0 into eBay:baseline_resync Nov 26, 2024
24 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.

5 participants