feat(server): Move bumpup logic out of FindInternal #4877
Conversation
mkaruza
force-pushed
the
mkaruza/github#4775
branch
2 times, most recently
from
2c7853d to
1820f14
Compare
| serialization_latch_.Wait(); | ||
| auto bump_it = db.prime.BumpUp(it, PrimeBumpPolicy{}); | ||
| if (bump_it != it) { // the item was bumped | ||
| ++events_.bumpups; |
There was a problem hiding this comment.
Lets add to our replication,snapshot pytests that we have for cache mode to check the bumpup counter, make sure that it is bigger than 0, to make sure we are still bumping items
There was a problem hiding this comment.
I have added pytest that checks few conditions for bumpups
|
we have this comment in MGET command |
|
please run this branch serveral times in regression tests github |
There was a problem hiding this comment.
Pull Request Overview
This PR moves the bumpup logic out of FindInternal and into OnCbFinish to skip bumping keys that no longer exist when the callback triggers.
- Updated test expectations in string_family_test.cc to reflect the new bumpup counts.
- Introduced FetchedItemKey and modified fetched_items_ to handle tuples.
- Refactored bumpup logic in DbSlice: removed immediate bumping in FindInternal and added deferred bumping in OnCbFinish.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/server/string_family_test.cc | Updated bumpup count assertions to align with new logic |
| src/server/db_slice.h | Introduced FetchedItemKey and adjusted fetched_items_ type |
| src/server/db_slice.cc | Moved bumpup operations from FindInternal to OnCbFinish and updated PrimeBumpPolicy |
Comments suppressed due to low confidence (2)
src/server/string_family_test.cc:287
- Since bumps1 is a size_t, the check EXPECT_GE(bumps1, 0) is always true. Consider removing it or adding a comment to explain its purpose if kept for clarity.
EXPECT_GE(bumps1, 0);
src/server/db_slice.cc:559
- Ensure that deferring the bump logic by simply inserting into fetched_items_ is fully aligned with the intended behavior, and document that the actual bump processing is now handled in OnCbFinish.
fetched_items_.insert({res.it->first.HashCode(), cntx.db_index, key});
Bumpup logic is moved to OnCbFinish. Previously keys which are going to be delete were also bumped up but with this change if key doesn't exists on callback we will skip it. Closes #4775 Signed-off-by: mkaruza <mario@dragonflydb.io>
Due random behavour of keys we should expect result of bumpups be in range of [0,10].
mkaruza
force-pushed
the
mkaruza/github#4775
branch
from
1820f14 to
b6e1b5c
Compare
| for (const auto& [key_hash, db_index, key] : moved_fetched_items_) { | ||
| auto& db = *db_arr_[db_index]; | ||
|
|
||
| auto predicate = [&key](const PrimeKey& key_) { return key_ == key; }; |
There was a problem hiding this comment.
you do not comparing to key, you can just return true here.
The point is that it's not critical that once in a billion chance you have a collision and you bump up something else
so you can remove key from fetched_items_ tuple
There was a problem hiding this comment.
I'll remove and add extra comment for this decision.
Bumpup logic is moved to OnCbFinish. Previously keys which are going to
be delete were also bumped up but with this change if key doesn't exists
on callback we will skip it.
Closes #4775