Skip to content

Wait for prefetch complete before freeing pg_iter - #321

Merged
koujl merged 1 commit into
eBay:mainfrom
koujl:prefetch-fix
Jul 11, 2025
Merged

Wait for prefetch complete before freeing pg_iter#321
koujl merged 1 commit into
eBay:mainfrom
koujl:prefetch-fix

Conversation

@koujl

@koujl koujl commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@koujl
koujl force-pushed the prefetch-fix branch 3 times, most recently from 6b9e3ec to 9f17292 Compare July 8, 2025 08:13
@codecov-commenter

codecov-commenter commented Jul 8, 2025

Copy link
Copy Markdown

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

Codecov Report

❌ Patch coverage is 48.57143% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.75%. Comparing base (1746bcc) to head (17b4080).
⚠️ Report is 168 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/homestore_backend/pg_blob_iterator.cpp 50.74% 22 Missing and 11 partials ⚠️
...ib/homestore_backend/replication_state_machine.cpp 0.00% 3 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
- Coverage   63.15%   60.75%   -2.41%     
==========================================
  Files          32       35       +3     
  Lines        1900     4072    +2172     
  Branches      204      492     +288     
==========================================
+ Hits         1200     2474    +1274     
- Misses        600     1333     +733     
- Partials      100      265     +165     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@koujl
koujl marked this pull request as ready for review July 8, 2025 09:06
@koujl
koujl requested a review from Copilot July 10, 2025 06:04

This comment was marked as outdated.

@koujl
koujl requested a review from Copilot July 10, 2025 06:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that the PGBlobIterator’s in-flight prefetch operations are properly drained before the iterator is freed, and it refactors iterator internals for naming consistency and thread-safety.

  • Introduce PGBlobIterator::stop() to wait for and drain pending prefetches before deletion
  • Rename iterator members (cur_obj_id_, group_id_, pg_id_, etc.) and add const correctness
  • Minor logging tweaks (removed trailing commas/newlines) and bump Conan package version

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/lib/homestore_backend/replication_state_machine.cpp Call stop() on the PGBlobIterator before deleting it; fix member access in logs
src/lib/homestore_backend/replication_message.hpp Remove trailing newline from SyncMessageHeader::to_string() output
src/lib/homestore_backend/pg_blob_iterator.cpp Refactor iterator internals: rename members, add stop() and locks, adjust const
src/lib/homestore_backend/hs_homeobject.hpp Update class declaration for PGBlobIterator to match refactoring
conanfile.py Bump version from 2.6.2 to 2.6.3
Comments suppressed due to low confidence (2)

src/lib/homestore_backend/replication_message.hpp:102

  • [nitpick] Removed the trailing newline from the to_string() output, which might cause log entries to run together; consider preserving the newline in the formatted string or adding it at the logging site to maintain readability.
        return fmt::format("magic={:#x} version={} msg_type={} payload_size={} payload_crc={} header_crc={}",

src/lib/homestore_backend/pg_blob_iterator.cpp:435

  • Added a new stop() method to wait for and drain inflight prefetch operations; consider adding unit tests to verify correct stopping behavior and ensure no memory leaks or deadlocks occur.
void HSHomeObject::PGBlobIterator::stop() {

Comment thread src/lib/homestore_backend/pg_blob_iterator.cpp
Comment thread src/lib/homestore_backend/pg_blob_iterator.cpp
@koujl
koujl requested review from Besroy, xiaoxichen and yuwmao July 10, 2025 06:36
if (hit_error) {
builder_.Clear();
// drain the prefetched data before return which destroy the iterator.
for (auto& blob : prefetched_blobs) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did I understand correctly: if hit error here, let nuraft call clear_snapshot_sync_ctx to trigger 'stop'?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct. The cleanup part will be done by stop().

@xiaoxichen xiaoxichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks for the fix

@koujl
koujl merged commit 3a5ca44 into eBay:main Jul 11, 2025
25 checks passed
@koujl
koujl deleted the prefetch-fix branch July 11, 2025 03:21
@Besroy

Besroy commented Jul 11, 2025

Copy link
Copy Markdown
Contributor

#318

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