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

Baseline Resync Enhancement #244

Open
wants to merge 2 commits into
base: baseline_resync
Choose a base branch
from

Conversation

yuwmao
Copy link
Contributor

@yuwmao yuwmao commented Dec 17, 2024

This PR enhances baseline resync:

  1. Address blob duplication issues caused by resync, it mainly happens when resend snapshot mesg during baseline resync and apply log after snapshot completion. This helps avoid unnecessary GC due to duplicated data.
    This mechanism is effective only for duplicated blobs. Since we do not record the blks of the shardInfo stored in the data service, we are unable to skip data writes for duplicated shards.
  2. Reset PG and context at the beginning of the baseline resync if pg is already present.

@yuwmao
Copy link
Contributor Author

yuwmao commented Dec 17, 2024

FYI, corresponding homestore change

@@ -379,17 +379,6 @@ void HSHomeObject::on_shard_message_commit(int64_t lsn, sisl::blob const& h, hom
local_create_shard(shard_info, v_chunk_id, blkids.chunk_num(), blkids.blk_count());
if (ctx) { ctx->promise_.setValue(ShardManager::Result< ShardInfo >(shard_info)); }

// update pg's total_occupied_blk_count
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redundant, these codes are already put into the local_create_shard.

@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2024

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

Codecov Report

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

Project coverage is 62.97%. Comparing base (55efbde) to head (eea3b8b).
Report is 3 commits behind head on baseline_resync.

Files with missing lines Patch % Lines
src/lib/homestore_backend/hs_blob_manager.cpp 35.71% 6 Missing and 3 partials ⚠️
src/lib/homestore_backend/hs_pg_manager.cpp 0.00% 4 Missing ⚠️
...ib/homestore_backend/replication_state_machine.cpp 0.00% 3 Missing ⚠️
...lib/homestore_backend/snapshot_receive_handler.cpp 0.00% 2 Missing ⚠️
src/lib/homestore_backend/index_kv.cpp 50.00% 1 Missing ⚠️

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

Additional details and impacted files
@@                 Coverage Diff                 @@
##           baseline_resync     #244      +/-   ##
===================================================
- Coverage            65.52%   62.97%   -2.55%     
===================================================
  Files                   33       33              
  Lines                 2271     2385     +114     
  Branches               254      271      +17     
===================================================
+ Hits                  1488     1502      +14     
- Misses                 659      753      +94     
- Partials               124      130       +6     

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

@yuwmao yuwmao force-pushed the baseline_resync branch 2 times, most recently from 556376d to f743e81 Compare December 23, 2024 07:03
@yuwmao yuwmao changed the title Blob duplication handling[Drafting] Baseline Resync Enhancement[Drafting] Dec 23, 2024
@yuwmao yuwmao force-pushed the baseline_resync branch 2 times, most recently from ce6769f to b07084c Compare December 24, 2024 07:14
@yuwmao yuwmao closed this Dec 24, 2024
@yuwmao yuwmao reopened this Dec 24, 2024
@yuwmao yuwmao requested review from koujl and xiaoxichen December 24, 2024 08:45
@yuwmao yuwmao changed the title Baseline Resync Enhancement[Drafting] Baseline Resync Enhancement Dec 24, 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

@@ -354,6 +360,13 @@ HSHomeObject::blob_put_get_blk_alloc_hints(sisl::blob const& header, cintrusive<
return folly::makeUnexpected(homestore::ReplServiceError::FAILED);
}

auto pg_iter = _pg_map.find(msg_header->pg_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: can we change this part to is_pg_present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we need to get the pg_iter as we are going to check index, so we can't use is_pg_present directly.

Copy link
Contributor Author

@yuwmao yuwmao Dec 25, 2024

Choose a reason for hiding this comment

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

Found there is already a pg_exists function, remove is_pg_present function.

This commit addresses blob duplication issues caused by resync, it mainly happens when resend snapshot mesg during baseline resync and apply log after snapshot completion. This helps avoid unnecessary GC due to duplicated data.
This mechanism is effective only for duplicated blobs. Since we do not record the blks of the `shardInfo` stored in the data service, we are unable to skip data writes for duplicated shards.
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