Skip to content

Conversation

@yamingk
Copy link
Contributor

@yamingk yamingk commented May 5, 2025

To reviewers:
Please kindly change setting hide whitespace and reload, a recent PR seems didn't apply clang-format on a few files.

There is only one line change in index_table.hpp which I commented in the PR.

Passed test against HomeStore consumer HomeObject.

conanfile.py (homeobject/2.3.16): RUN: cmake --build "/tmp/source/HomeObject/build/Debug" --target test -- -j104
Running tests...
Test project /tmp/source/HomeObject/build/Debug
      Start  1: HomestoreTestPg
 1/11 Test  #1: HomestoreTestPg ................................   Passed  273.15 sec
      Start  2: HomestoreTestShard
 2/11 Test  #2: HomestoreTestShard .............................   Passed  133.87 sec
      Start  3: HomestoreTestBlob
 3/11 Test  #3: HomestoreTestBlob ..............................   Passed  226.44 sec
      Start  4: HomestoreTestMisc
 4/11 Test  #4: HomestoreTestMisc ..............................   Passed   39.42 sec
      Start  5: HomestoreTestReplaceMember
 5/11 Test  #5: HomestoreTestReplaceMember .....................   Passed   42.52 sec
      Start  6: HomestoreTestReplaceMemberWithBaselineResync
 6/11 Test  #6: HomestoreTestReplaceMemberWithBaselineResync ...   Passed   31.49 sec
      Start  7: HomestoreResyncTestWithFollowerRestart
 7/11 Test  #7: HomestoreResyncTestWithFollowerRestart .........   Passed  140.71 sec
      Start  8: HomestoreResyncTestWithLeaderRestart
 8/11 Test  #8: HomestoreResyncTestWithLeaderRestart ...........   Passed   52.13 sec
      Start  9: HeapChunkSelectorTest
 9/11 Test  #9: HeapChunkSelectorTest ..........................   Passed    0.10 sec
      Start 10: MemoryTestCPU
10/11 Test #10: MemoryTestCPU ..................................   Passed    1.55 sec
      Start 11: MemoryTestIO
11/11 Test #11: MemoryTestIO ...................................   Passed    1.91 sec

100% tests passed, 0 tests failed out of 11

Total Test time (real) = 943.32 sec 

auto cpg = cp_mgr().cp_guard();
Btree< K, V >::destroy_btree(cpg.context(cp_consumer_t::INDEX_SVC));
m_sb.destroy();
m_sb_buffer->m_valid = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

online this change in this file

@codecov-commenter
Copy link

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

Codecov Report

Attention: Patch coverage is 37.78802% with 135 lines in your changes missing coverage. Please review.

Project coverage is 66.85%. Comparing base (1a0cef8) to head (4526ff9).
Report is 190 commits behind head on master.

Files with missing lines Patch % Lines
src/include/homestore/index/index_table.hpp 36.05% 98 Missing and 35 partials ⚠️
src/lib/index/index_service.cpp 50.00% 1 Missing ⚠️
src/lib/index/wb_cache.cpp 85.71% 0 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #714       +/-   ##
===========================================
+ Coverage   56.51%   66.85%   +10.33%     
===========================================
  Files         108      109        +1     
  Lines       10300    11946     +1646     
  Branches     1402     1666      +264     
===========================================
+ Hits         5821     7986     +2165     
+ Misses       3894     3126      -768     
- Partials      585      834      +249     

☔ 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.

auto const& sb = r_cast< MetaIndexBuffer* >(buf.get())->m_sb;
if (!sb.is_empty()) { meta_service().update_sub_sb(buf->m_bytes, sb.size(), sb.meta_blk()); }
auto const sb_buf = r_cast< MetaIndexBuffer* >(buf.get());
if (sb_buf->m_valid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should ideally assert, so may be add a todo. After the btree fix, we can add assert.

Copy link
Contributor Author

@yamingk yamingk May 6, 2025

Choose a reason for hiding this comment

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

I think the purpose here for the check is to avoid flushing some m_sb that is destroyed before cp arrives. e.g. it should be fine that when cp arrives, the related dirty sb has been destroyed, right? Alternatively when it is being destroyed, we can have cp consumer to remove it from dirty list (CP has to provide such api though), however that seems to be unnecessary, imo.

@yamingk yamingk merged commit f30f0d4 into eBay:master May 6, 2025
26 checks passed
@yamingk yamingk deleted the yk_index_wb_destroy branch May 6, 2025 02:04
@yamingk yamingk linked an issue May 7, 2025 that may be closed by this pull request
hkadayam pushed a commit to hkadayam/HomeStore that referenced this pull request Aug 7, 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.

Index Table destroy path is racing with wb_cache cp_flush .

3 participants