Conversation
4ad7814 to
e2d3391
Compare
e2d3391 to
1d2aef4
Compare
| ${HOMESTORE_OBJECTS} | ||
| ) | ||
| target_link_libraries(homestore ${COMMON_DEPS}) | ||
| target_compile_definitions(homestore PRIVATE _PRERELEASE) |
There was a problem hiding this comment.
why we need this _PRERELEASE here? I think target_compile_definitions(test_index_chunk_selector PRIVATE _PRERELEASE) in tests/CmakeLists.txt is enough for the new test case
There was a problem hiding this comment.
Because otherwise the code won't compile.
There was a problem hiding this comment.
adding _PRERELEASE unconditionally to the homestore library itself means every build (including production Release builds) will now permanently carry _PRERELEASE: extra fault-injection checkpoints, extra assert branches, and extra debug fields get baked into the production library. That's a much bigger behavioral change than "just make the new test compile." pls correct me if I misunderstand something.
There was a problem hiding this comment.
You are right. I added _PRERELEASE just to make this compile and run. Please suggest a better place/way to enable _PRERELEASE as I didn't really find how it is enabled in 7.x. I presume it is not the same as Debug build.
| virtual void recover(sisl::byte_view sb) = 0; | ||
|
|
||
| /// Remove entries for a specific chunk from wbc | ||
| virtual void evict_chunk_blkids(const Chunk& chunk) = 0; |
There was a problem hiding this comment.
NIT: if this a time-consuming task, I would suggest make it return a sisl::async::task < void >, so that we can use co_wait to wait for the completion asyncronously?
There was a problem hiding this comment.
It is a time-consuming task, we call it from HomeBlocks within iomanager.run_on_forget(), check the other half of this PR: eBay/HomeBlocks#179
We don't need to wait for it as it does all the job itself.
There was a problem hiding this comment.
IMO, if we do nott need wait for it and it is a time-consuming task, then it would be better to return a sisl::async::task < void >, so that we can use detach to run this time-consuming task asyncronously and not block the calling thread
There was a problem hiding this comment.
iomanager.run_on_forget() is also async, it does not block the caller and I am not a big fan of detach() personally. So this is basically a stylistic matter, unless there is something I don't know about sisl::async.
Please elaborate how this would be better for this particular case.
| const auto cid = chunk_id(); | ||
|
|
||
| for (blk_num_t blk_num{}; blk_num < total_blks; ++blk_num) { | ||
| blk_id blkid{blk_num, 1, cid}; |
There was a problem hiding this comment.
I am not quite sure if we need a lock to guarantee the thread safety of this function
There was a problem hiding this comment.
Can you please elaborate? What exactly needs to be protected?
The chunk is not visible to chunk selector yet and SimpleCache seems to be thread safe.
There was a problem hiding this comment.
honestly this is just my intuition . if a thread is calling free_blk or reset blk_allocator, another thread is calling foreach_allocated_blk, not sure this will bring some thread safety issue.
There was a problem hiding this comment.
Nothing should touch (or even see) this chunk while chunk selector is in charge.
|
@szmyd @shosseinimotlagh Please if you can ensure the upgrade path and compatibility are properly discussed. NuObject is moving to V7 mostly to intake the folly and co-routine removal, which is still a persistent-compatible version (meaning , no data migration is needed upgrading from v6 to v7). With the potential path forward of this feature, it is unlikely to be compatible with v7 as of now. I would suggest it should be in V8 with proper feature bit. Also decisions regarding how V6 can upgrade to V8 should be discussed. |
It's my understanding that we only use v3 (NuBlox 1.x/2.0) and v7 (NuObject) at the moment. We're currently working on v8 for NuBlox 2.1; so where does v6 come into play? |
|
@szmyd sorry you are right, I mess up v7 vs v6.. Yes I am talking about v7 ->v8 compatibility |
There was a problem hiding this comment.
🟡 Changes recommended
Production cleanup is not wired into chunk release, and the build and test lifecycle contain blocking defects.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Introduces chunk-level WBC eviction primitives intended to prevent stale cache entries when index chunks are reused.
Changes:
- Adds allocated-block enumeration and WBC chunk eviction APIs.
- Adds a deterministic chunk-reuse regression test.
- Updates test helpers, build configuration, and package version.
File summaries
| File | Description |
|---|---|
conanfile.py |
Bumps version to 8.2.1. |
src/CMakeLists.txt |
Enables prerelease compilation. |
src/include/homestore/index/wb_cache_base.hpp |
Exposes chunk eviction API. |
src/lib/device/chunk.cpp |
Enumerates allocated blocks. |
src/lib/device/chunk.h |
Declares block enumeration. |
src/lib/index/wb_cache.cpp |
Implements chunk-wide eviction. |
src/lib/index/wb_cache.hpp |
Declares eviction override. |
src/tests/CMakeLists.txt |
Registers the regression test. |
src/tests/btree_helpers/btree_test_helper.hpp |
Makes flip arguments explicit arrays. |
src/tests/test_common/homestore_test_common.hpp |
Makes flip arguments explicit arrays. |
src/tests/test_index_chunk_selector.cpp |
Adds the chunk-reuse regression test. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| target_link_libraries(homestore ${COMMON_DEPS}) | ||
| target_compile_definitions(homestore PRIVATE _PRERELEASE) |
This change makes chunk release asynchronous and defers returning the chunk to the free pool until cleanup is complete.
New flow:
This PR also addresses this ticket SDSTOR-25404