Skip to content

feat(server): Make DbSlice consumer an interface#7253

Draft
dranikpg wants to merge 6 commits intodragonflydb:mainfrom
dranikpg:dbslice-consumer-interface
Draft

feat(server): Make DbSlice consumer an interface#7253
dranikpg wants to merge 6 commits intodragonflydb:mainfrom
dranikpg:dbslice-consumer-interface

Conversation

@dranikpg
Copy link
Copy Markdown
Contributor

@dranikpg dranikpg commented May 2, 2026

  1. Introduce ChangeConsumerInterface to allow for more interactions with registered consumers
  2. Change latch to be only a guard to prevent removal during traversal of change callbacks
  3. Use consumer interface to determine if serialization is going on and create proper functions for the db_slice
  4. Create a serializer_base_test test that shows how eviction could have happened earlier before a delayed entry was written, effectively cancelling the eviction

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg
Copy link
Copy Markdown
Contributor Author

dranikpg commented May 3, 2026

augment review

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented May 3, 2026

🤖 Augment PR Summary

Summary: This PR refactors DbSlice change notification from a callback+id model to a consumer interface, enabling richer interactions with snapshot/serialization code.

Changes:

  • Introduced DbSlice::ChangeConsumerInterface and converted registered change listeners from (version, callback) to consumer pointers.
  • Replaced the previous serialization latch usage with a new change_cb_latch_ intended to guard safe traversal/removal of registered consumers.
  • Added DbSlice helpers (WillBlockOnJournalWrite, WaitForUnblockedJournalWrites) that query consumers for blocked-bucket state to avoid journal-write preemption during sensitive traversals.
  • Updated cluster slot flushing to register a temporary consumer object (via a CallbackConsumer) and unregister it after the fiber finishes.
  • Updated SerializerBase to implement the new consumer interface and report/await bucket dependency blocking via BucketDependencies.
  • Updated tests to use the new consumer API and added serializer regression coverage around delayed-entry eviction/expiry interactions.

Technical Notes: Snapshot versions are now stored on the consumer object itself (snapshot_version_), and the slice consults consumers to decide whether journaling can safely proceed without being rejected by blocked serialization.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

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

Review completed. 4 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/server/db_slice.cc
Comment thread src/server/db_slice.cc
Comment thread src/server/db_slice.h
public:
// Consumer of bucket change events than can be registered inside the slice.
// It also includes additional methods for interfacing with snapshots and migrations.
struct ChangeConsumerInterface {
Copy link
Copy Markdown

@augmentcode augmentcode Bot May 3, 2026

Choose a reason for hiding this comment

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

src/server/db_slice.h:89: ChangeConsumerInterface is used as a polymorphic base, but it doesn’t declare a virtual destructor, which can cause UB if a consumer is ever deleted through a ChangeConsumerInterface* (easy to do with interfaces). Consider adding a virtual default destructor to make ownership patterns safer.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

TEST_current_time_ms = TEST_current_time_ms + 100;
for (unsigned i = 0; i < kKeys; i++)
EXPECT_THAT(Run({"GET", absl::StrCat("key:", i)}), ArgType(RespExpr::NIL));
Run({"SET", absl::StrCat("key:", i), "V"});
Copy link
Copy Markdown

@augmentcode augmentcode Bot May 3, 2026

Choose a reason for hiding this comment

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

src/server/serializer_base_test.cc:415: In DelayedAllDeleted, the loop after advancing time uses SET instead of asserting the keys were actually expired/deleted, so the test may no longer exercise the “bucket becomes empty due to expiry deletion” scenario described in the header comment. If the goal is to validate that expiry-driven deletions flush delayed reads, it may be worth reintroducing an explicit assertion/trigger of expiry deletion here.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

dranikpg added 2 commits May 3, 2026 12:25
Signed-off-by: Vlad <vlad@dragonflydb.io>
@dranikpg dranikpg force-pushed the dbslice-consumer-interface branch from 309629b to 48ec103 Compare May 4, 2026 05:56
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.

1 participant