feat(server): Support CLIENT TRACKING subcommand (2/2) #2280

theyueli · 2023-12-09T00:33:44Z

This is part two that implements the logic which notifies tracking clients by sending invalidation messages.

Once part 1 is merged, this PR will gets smaller (it includes part 1 currently)

Functional tests have also been included, which includes a fix for #2304

The client tracking state is set by CLIENT TRACKING subcommand as well as upon client disconnection.

Track the keys of a readonly command by maintaining mapping that maps keys to the sets of tracking clients.

Send invalidation messages to clients when their tracked keys are updated.

Signed-off-by: Yue Li <[email protected]>

src/facade/dragonfly_connection.cc

dranikpg · 2023-12-09T07:22:13Z

src/server/db_slice.cc

+    auto& client_set = client_tracking_map_[key];
+    DVLOG(2) << "Garbage collect clients that are no longer tracking... ";
+    auto is_closed_or_not_tracking = [](const facade::Connection::WeakRef& p) {
+      return (p.IsExpired() || (!p.Get()->IsTrackingOn()));


Technically, this Get here is unsafe. In practice the the other thread can never be so fast to delete the connection faster than reading a variable...

Better do it from cb, as you drop them below either way

src/server/db_slice.cc

dranikpg · 2023-12-09T07:25:04Z

src/server/db_slice.cc

+    DVLOG(2) << "Number of clients left: " << client_set.size();
+
+    if (!client_set.empty()) {
+      auto cb = [key, client_set](unsigned idx, util::ProactorBase*) {


Better use [client_set = std::move(client_set)]

dranikpg · 2023-12-09T07:28:10Z

src/server/db_slice.cc

+
+void DbSlice::SendInvalidationTrackingMessage(const std::string_view& key) {
+  if (client_tracking_map_.find(key) != client_tracking_map_.end()) {
+    // notify all the clients.


Some nits:
You can pass string_view by value
You can store the it = find(key) to avoid reading again with [key]

src/server/db_slice.cc

dranikpg · 2023-12-09T14:26:02Z

src/server/db_slice.cc

-    absl::erase_if(client_set, is_closed_or_not_tracking);
-    DVLOG(2) << "Number of clients left: " << client_set.size();


I thought you do this to reduce the number of traversals (because each thread currently iterates over the whole set), but I assume this optimization is mostly not worth it

adiholden · 2023-12-10T12:11:01Z

src/server/db_slice.cc

            PerformDeletion(it, shard_owner(), db_ptr.get());
+            SendInvalidationTrackingMessage(key);


why dont you make PerformDeletion function of DbSlice? I dont like it that we need to remember to call SendInvalidationTrackingMessage whenever we call PerformDeletion

ok, will do.

why dont you make PerformDeletion function of DbSlice? I dont like it that we need to remember to call SendInvalidationTrackingMessage whenever we call PerformDeletion

done, please take another look. I like this change, saved a lot of code duplication.

src/server/server_family.cc

…ion message within the function.

src/server/server_family.cc

adiholden · 2023-12-12T10:12:05Z

src/facade/dragonfly_connection.h

@@ -116,6 +116,11 @@ class Connection : public util::Connection {
    util::fb2::BlockingCounter bc;  // Decremented counter when processed
  };

+  struct InvalidationMessage {
+    std::string_view key;


I believe you should use a string here and not string_view.
Who is the owner of this string and how do you know it is alive until you send the message?

adiholden · 2023-12-12T10:46:32Z

src/server/db_slice.h

@@ -338,6 +336,12 @@ class DbSlice {
  // Track keys for the client represented by the the weak reference to its connection.
  void TrackKeys(const facade::Connection::WeakRef&, const ArgSlice&);

+  // Send invalidation message to the clients that are tracking the change to a key.
+  void SendInvalidationTrackingMessage(std::string_view key);


yes, fixed.

src/server/db_slice.cc

adiholden · 2023-12-12T11:26:35Z

src/server/db_slice.cc

@@ -1233,6 +1172,8 @@ size_t DbSlice::EvictObjects(size_t memory_to_free, PrimeIterator it, DbTable* t
    return current < used_memory_start ? used_memory_start - current : 0;
  };

+  string_view key;


adiholden · 2023-12-12T11:28:51Z

src/server/db_slice.cc

    RecordExpiry(cntx.db_index, tmp_key);
  }

  auto obj_type = it->second.ObjType();
  if (doc_del_cb_ && (obj_type == OBJ_JSON || obj_type == OBJ_HASH)) {
-    if (tmp_key.empty())


I believe you can revert the changes in this function

- mock the function for sending invalidation message to avoid test crash due to lack of real listener in the testing framework.

dranikpg

The testing approach is the same as I had in mind. Please note there are still some unresolved comments from Adi

dranikpg · 2023-12-16T18:41:04Z

src/server/server_family_test.cc

+  // case 7. test multi command
+  Run({"MGET", "X", "Y", "Z"});
+  pp_->at(1)->Await([&] { return Run({"MSET", "X", "1", "Y", "2"}); });
+  EXPECT_EQ(GetInvalidationMessage("IO0", 3).key, "X");
+  EXPECT_EQ(GetInvalidationMessage("IO0", 4).key, "Y");


I'd make the test a little more sophisticated. Let's try multiple keys on all shards, so that we don't even know the order

even with this test, the order of arrival is different now, btw, is there a convenient test function that checks the completeness disregarding the order?

added a few more keys, how does this look?

dranikpg · 2023-12-16T18:42:24Z

src/facade/dragonfly_connection.cc

+    std::vector<string_view> keys{msg.key};
+    rbuilder->SendStringArr(keys);


string_view[] keys = {msg.key} SendStringArr(keys)

does the same without allocations

This doesn't compile

/home/yli/git/dragonfly-rebase/src/facade/dragonfly_connection.cc:303:21: error: structured binding declaration cannot have type ‘std::string_view’ {aka ‘std::basic_string_view<char>’} 303 | std::string_view[] keys = {msg.key}; | ^~ /home/yli/git/dragonfly-rebase/src/facade/dragonfly_connection.cc:303:21: note: type must be cv-qualified ‘auto’ or reference to cv-qualified ‘auto’ /home/yli/git/dragonfly-rebase/src/facade/dragonfly_connection.cc:303:21: error: empty structured binding declaration /home/yli/git/dragonfly-rebase/src/facade/dragonfly_connection.cc:303:24: error: expected initializer before ‘keys’ 303 | std::string_view[] keys = {msg.key}; | ^~~~ /home/yli/git/dragonfly-rebase/src/facade/dragonfly_connection.cc:304:29: error: ‘keys’ was not declared in this scope 304 | rbuilder->SendStringArr(keys); | ^~~~

try string_view keys[] = {msg.key};
It thinks you're using structured binding, using the brackets on the left side is C#/Java syntax

this works well, thanks!

theyueli · 2023-12-16T18:49:44Z

The testing approach is the same as I had in mind. Please note there are still some unresolved comments from Adi

yes, i plan to add more tests now after verifying the mock methods with you guys.

adiholden · 2023-12-17T08:49:46Z

src/server/server_family_test.cc

@@ -200,4 +200,52 @@ TEST_F(ServerFamilyTest, ClientPause) {
  EXPECT_GT((absl::Now() - start), absl::Milliseconds(50));
 }

+TEST_F(ServerFamilyTest, ClientTracking) {


I would create different unit test for different test cases.
I think that it is great that we have this in unit tests with mocking but we must add pytest to make sure this command works well to check the functionality without the mocking

that's fine with me too. Currently i see different styles mixed in our code: sometimes all cases went into one function (e.g. ClientPause here, and many other redis commands' tests), sometimes separated smaller cases (e.g. SlowLog here)

Signed-off-by: Yue Li <[email protected]>

theyueli · 2023-12-20T09:17:08Z

all the comments have been addressed. Please take another look. @dranikpg @adiholden @chakaz

adiholden · 2023-12-20T09:34:46Z

src/server/db_slice.cc

@@ -1049,18 +1051,15 @@ DbSlice::ItAndExp DbSlice::ExpireIfNeeded(const Context& cntx, PrimeIterator it)
    return {it, expire_it};

  string tmp_key_buf;
-  string_view tmp_key;
+  string_view tmp_key = it->first.GetSlice(&tmp_key_buf);


please revert the changes here to be as before. you did not changed this function

adiholden · 2023-12-20T11:46:01Z

src/server/server_family.cc

+    listener->TraverseConnections(cb);
+  }
+}
+
 void ServerFamily::FlushDb(CmdArgList args, ConnectionContext* cntx) {
  DCHECK(cntx->transaction);
  Drakarys(cntx->transaction, cntx->transaction->GetDbIndex());


I think we should clear client_tracking_map_ in flush

this is not needed as Drakarys will finally end up calling PerformDeletion for each key, which will get the tracking table removed at the end.

FushDb calls PerformDeletion only for external keys (ssd).
If we would have called PerformDeletion for all keys in flushdb the client would get the invalidation message twice, one for all keys and one for each key in the database.
If we will have client tracking with tiering I believe the behaviour of flushall will be wrong. This is why I wrote above to get the invalidation message out of PerformDeletion that should just free the data and update table counters

i see what you meant, I just added another commit which clear client_tracking_map in FlushDB of DbSlice:

287c3f5

This shall fix both problems:

FlushDb and FlushAll of ServerFamily will end up clearing the client tracking map

For SSD tiering, since the map has been cleared before PerformDeletion gets called, notification will not be sent out twice.

adiholden · 2023-12-20T12:00:34Z

src/server/db_slice.cc

+  SendInvalidationTrackingMessage(key);
+}
+
+void DbSlice::PerformDeletion(PrimeIterator del_it, EngineShard* shard, DbTable* table) {


After reviewing this more times and reviewing other PR in this file I now understand that actually PerformDeletion should not be a member of DbSlice as I asked before. Rather we should have in DbSlice and function DeleteInternal which will write to journal, do the doc_del_cb_ call, SendInvalidationTrackingMessage
This function will be called from all the flows except the flush flows in which we dont need to do this changes and there we will call PerformDeletion which will be static function.

I believe this will fix several bugs we have that we forget to call the doc_del_cb_ and the journal change writes

This kind of refactoring is out of the scope for this PR. We will do it in another one once this gets merged.

Yes the refactoring can be done in a seprate PR. But as I wrote in another comment I believe that sending Invalidation message from PerformDeletion is incorrect for the flow of flush all.

OK, please see if my reply above will resolve this issue as well.

Signed-off-by: Yue Li <[email protected]>

adiholden · 2023-12-21T06:58:44Z

@theyueli you are going to commit the pytests to this PR correct?

…-invalidate

theyueli · 2023-12-21T12:04:09Z

@theyueli you are going to commit the pytests to this PR correct?

Probably in a separate PR.

theyueli added 6 commits December 7, 2023 03:56

adding client tracking state for dragonfly connection.

273d5a9

The client tracking state is set by CLIENT TRACKING subcommand as well as upon client disconnection.

Implement CLIENT TRACKING subcommand.

55334c7

Track the keys of a readonly command by maintaining mapping that maps keys to the sets of tracking clients.

clean

c4ecdcc

clean

1bf7c40

review comments

5378a78

Implement CLIENT TRACKING subcommand.

11abbec

Send invalidation messages to clients when their tracked keys are updated.

theyueli added the enhancement New feature or request label Dec 9, 2023

theyueli requested review from romange, chakaz and dranikpg December 9, 2023 00:33

theyueli self-assigned this Dec 9, 2023

theyueli requested a review from adiholden December 9, 2023 00:34

theyueli added 2 commits December 8, 2023 23:20

Merge branch 'main' into ct-invalidate

3dd10ee

Signed-off-by: Yue Li <[email protected]>

clean up

1433f98

Signed-off-by: Yue Li <[email protected]>

theyueli commented Dec 9, 2023

View reviewed changes

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

theyueli mentioned this pull request Dec 9, 2023

feat(server): support client side tracking (resp3 protocol only) #2233

Closed

dranikpg reviewed Dec 9, 2023

View reviewed changes

theyueli added 3 commits December 9, 2023 02:24

review comments

8c6d974

review comments

5de9465

remove unnecessary garbage collection (key gets erased anyway)

56ed820

dranikpg reviewed Dec 9, 2023

View reviewed changes

adiholden reviewed Dec 10, 2023

View reviewed changes

src/server/server_family.cc Show resolved Hide resolved

make PerformDeletion a member function of DbSlice, and send invalidat…

e2d8fa2

…ion message within the function.

adiholden reviewed Dec 12, 2023

View reviewed changes

src/server/server_family.cc Show resolved Hide resolved

adiholden reviewed Dec 12, 2023

View reviewed changes

src/server/db_slice.cc Show resolved Hide resolved

adiholden reviewed Dec 12, 2023

View reviewed changes

theyueli added 2 commits December 12, 2023 15:36

review comments

d8b145f

- adding functional (some) tests for client tracking.

d5ba74c

- mock the function for sending invalidation message to avoid test crash due to lack of real listener in the testing framework.

dranikpg reviewed Dec 16, 2023

View reviewed changes

adiholden reviewed Dec 17, 2023

View reviewed changes

theyueli and others added 6 commits December 19, 2023 14:11

review comments

b5ccf1a

review comments and more functional tests

802160e

Merge branch 'main' into ct-invalidate

a3fb9e6

Signed-off-by: Yue Li <[email protected]>

make pre-commit hook happy

5c4c190

make compiler happy

584916a

try to fix functional test failure

e2dab2e

adiholden reviewed Dec 20, 2023

View reviewed changes

review comments

caa4af6

adiholden reviewed Dec 20, 2023

View reviewed changes

Merge branch 'main' into ct-invalidate

6826910

Signed-off-by: Yue Li <[email protected]>

theyueli added 2 commits December 21, 2023 03:57

review comments

287c3f5

Merge branch 'ct-invalidate' of github.com:theyueli/dragonfly into ct…

3a40aec

…-invalidate

adiholden approved these changes Dec 21, 2023

View reviewed changes

theyueli merged commit 6905389 into dragonflydb:main Dec 21, 2023

theyueli deleted the ct-invalidate branch December 21, 2023 12:40

theyueli mentioned this pull request Dec 21, 2023

bug: support client tracking message in functional test #2304

Closed

		absl::erase_if(client_set, is_closed_or_not_tracking);
		DVLOG(2) << "Number of clients left: " << client_set.size();

		PerformDeletion(it, shard_owner(), db_ptr.get());
		SendInvalidationTrackingMessage(key);

		std::vector<string_view> keys{msg.key};
		rbuilder->SendStringArr(keys);

feat(server): Support CLIENT TRACKING subcommand (2/2) #2280

feat(server): Support CLIENT TRACKING subcommand (2/2) #2280

Uh oh!

Conversation

theyueli commented Dec 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dranikpg Dec 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dranikpg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theyueli commented Dec 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theyueli commented Dec 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

theyueli commented Dec 9, 2023 •

edited

Loading

dranikpg Dec 9, 2023 •

edited

Loading

theyueli Dec 21, 2023 •

edited

Loading