Skip to content

Add generic handle logic for non-data-op - #338

Merged
Besroy merged 1 commit into
eBay:mainfrom
Besroy:fix
Aug 28, 2025
Merged

Besroy merged 1 commit into
eBay:mainfrom
Besroy:fix

Conversation

@Besroy

@Besroy Besroy commented Aug 28, 2025 •

Copy link
Copy Markdown
Contributor

This pr avoids the false alarm of corrupted message when HS adds ctrl type log but HO is not updated

@Besroy
Besroy requested a review from koujl August 28, 2025 06:15
LOGI("pre_commit HS_CTRL_UPDATE_TRUNCATION_BOUNDARY log entry, lsn={}", lsn);
return true;
}

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.

IIUC the real issue before this commit was homestore::journal_type_t::HS_CTRL_UPDATE_TRUNCATION_BOUNDARY reaches L99 and got NPE somewhere?

Can we cleanup the L80 - L98 like

if (ctx->op_code() != homestore::journal_type_t::HS_DATA_LINKED) && (ctx->op_code() != homestore::journal_type_t::HS_DATA_INLINED)  {
        LOGI("pre_commit TO_STRING(ctx->op_code()) log entry, lsn={}", lsn);
        return true;
    }
else {
   assert(ctx->op_code() == journal_type_t::HS_DATA_LINKED || ctx->op_code() == journal_type_t::HS_DATA_INLINED);
       .....
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i am curious why we hit this issue until now. Theoretically, any HS_CTRL_UPDATE_TRUNCATION_BOUNDARY will hit this issue. Don`t we enable UPDATE_TRUNCATION_BOUNDARY in previous tests?

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.

This PR doesn't fix the crash issue; it simply reduces the log noise from corrupted messages due to unhandled control messages.
Thank you for your comment ! It's a great idea, and I've updated the code

@codecov-commenter

codecov-commenter commented Aug 28, 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 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 60.28%. Comparing base (1746bcc) to head (5ceb111).
⚠️ Report is 115 commits behind head on main.

Files with missing lines Patch % Lines
...ib/homestore_backend/replication_state_machine.cpp 80.00% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
- Coverage   63.15%   60.28%   -2.88%     
==========================================
  Files          32       35       +3     
  Lines        1900     4253    +2353     
  Branches      204      511     +307     
==========================================
+ Hits         1200     2564    +1364     
- Misses        600     1424     +824     
- 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.

@Besroy Besroy changed the title add pre commit handle logic for HS_CTRL_UPDATE_TRUNCATION_BOUNDARY Add generic handle logic for non-data-op Aug 28, 2025
@Besroy
Besroy merged commit 9bc58d9 into eBay:main Aug 28, 2025
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.

4 participants