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

theyueli · 2023-12-08T02:14:20Z

Implement CLIENT TRACKING subcommand.

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

Also include a transaction interface Refurbish() that allows the reuse of a transaction, which is used for tracking the keys involved in a readonly command.

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.

dranikpg · 2023-12-08T06:32:56Z

src/server/main_service.cc

@@ -1465,6 +1485,9 @@ void Service::Quit(CmdArgList args, ConnectionContext* cntx) {
    cntx->SendOk();
  using facade::SinkReplyBuilder;

+  // turn off tracking for this client


No need for it here,.this path calls OnClose

dranikpg · 2023-12-08T06:34:12Z

src/server/server_family.cc

@@ -1012,6 +1012,7 @@ string ServerFamily::GetReplicaMasterId() const {

 void ServerFamily::OnClose(ConnectionContext* cntx) {
  dfly_cmd_->OnClose(cntx);
+  cntx->conn()->SetClientTrackingSwitch(false);


I assume we had MainService:: OnClose where watch, pubsub and monitoring are cleaning up. Let's add it there so all of them are in one place

yes, good catch! I used correct location in the draft PR and but made a mistake here.

dranikpg · 2023-12-08T06:41:27Z

src/server/main_service.cc

+      return OpTrackKeys(t->GetOpArgs(shard), dfly_cntx, keys_to_track);
+    };
+
+    dfly_cntx->transaction->Refurbish();


Refurbish() should be after you use it with schedulesinglehop, please add a test with move, zinterstore etc (commands that have two hops)

DispatchCommand is not used for all commands, but we can ignore that for now

Still reminding that we can track keys when the transaction concludes and no "hop" is needed solely for this, but I also assume it's not a must for the first version

Refurbish() should be after you use it with schedulesinglehop, please add a test with move, zinterstore etc (commands that have two hops)

I'm wondering, the transaction probably has already been used before the tracking (e.g. in the previous command invoking?) and I need to do a refurbish here before doing my own singlehop?

Still reminding that we can track keys when the transaction concludes and no "hop" is needed solely for this, but I also assume it's not a must for the first version

Sounds good to me, I will look into this later.

Re 1: Ah, it's placed below the command invocation... yes, then refurbish needs to be above it

please add a test with move, zinterstore etc (commands that have two hops)

sounds good, I will include all the functional tests in the second PR, which will completes the implementation with invalidation logic.

dranikpg · 2023-12-08T06:42:28Z

src/server/main_service.cc

+  if ((cid->opt_mask() & CO::READONLY) && dfly_cntx->conn()->IsTrackingOn()) {
+    auto cb = [&](Transaction* t, EngineShard* shard) {
+      auto keys = t->GetShardArgs(shard->shard_id());
+      vector<string_view> keys_to_track{keys.begin(), keys.end()};


Why copy it into a vector and not use the span?

yes, fixed.

dranikpg · 2023-12-08T06:47:01Z

src/server/db_slice.cc

+    } else {
+      DVLOG(2) << "The key " << key << " exists in the client tracking table.";
+      DVLOG(2) << "Inserting client ID " << conn.GetClientId() << " into its tracking client set: ";
+      client_tracking_map_[key].insert(conn);


I know you have different output for now, but at the end you don't need this if

map[key].insert() works even if key is not in map

src/facade/dragonfly_connection.h

theyueli · 2023-12-09T00:34:40Z

@dranikpg i addressed all the comments, please take another look.

dranikpg · 2023-12-09T07:14:29Z

src/server/db_slice.cc

@@ -1338,4 +1338,19 @@ void DbSlice::ResetUpdateEvents() {
  events_.update = 0;
 }

+void DbSlice::TrackKeys(const facade::Connection::WeakRef& conn, const ArgSlice& keys) {
+  if (conn.IsExpired()) {


This is impossible, I'd do DCHECK(!conn.Expired()) (in the next pr)

This is impossible, I'd do DCHECK(!conn.Expired()) (in the next pr)

got it, will do it there.

adiholden · 2023-12-10T10:30:31Z

src/server/server_family.cc

+    return cntx->SendError(
+        "Client tracking is currently not supported for RESP2. Please use RESP3.");
+
+  ToUpper(&args[0]);


CmdArgParser parser(args);
bool is_on = false;
if (parser.HasNext()) {
pause_state = parser.ToUpper().Switch("ON", true, "OFF", false);
}
if (auto err = parser.Error(); err) {
return (*cntx)->SendError(err->MakeReply());
}

src/server/db_slice.h

theyueli added 2 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.

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

theyueli requested review from romange, chakaz, dranikpg and adiholden December 8, 2023 02:14

theyueli self-assigned this Dec 8, 2023

theyueli changed the title ~~feat(server): Support Client Tracking (1/2)~~ feat(server): Support CLIENT TRACKING subcommand (1/2) Dec 8, 2023

theyueli added 2 commits December 7, 2023 18:16

clean

c4ecdcc

clean

1bf7c40

dranikpg reviewed Dec 8, 2023

View reviewed changes

review comments

5378a78

dranikpg self-requested a review December 9, 2023 07:08

dranikpg approved these changes Dec 9, 2023

View reviewed changes

theyueli merged commit 64bbfc7 into dragonflydb:main Dec 9, 2023
7 checks passed

theyueli deleted the ct-capturing branch December 9, 2023 07:14

dranikpg reviewed Dec 9, 2023

View reviewed changes

theyueli mentioned this pull request Dec 9, 2023

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

Closed

adiholden reviewed Dec 10, 2023

View reviewed changes

src/server/db_slice.h Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theyueli commented Dec 8, 2023 •

edited

dranikpg Dec 8, 2023

theyueli Dec 8, 2023

dranikpg Dec 8, 2023

theyueli Dec 8, 2023 •

edited

dranikpg Dec 8, 2023

theyueli Dec 8, 2023

theyueli Dec 8, 2023 •

edited

dranikpg Dec 8, 2023

theyueli Dec 8, 2023

dranikpg Dec 8, 2023

theyueli Dec 8, 2023

dranikpg Dec 8, 2023

theyueli Dec 8, 2023

theyueli commented Dec 9, 2023

dranikpg Dec 9, 2023

theyueli Dec 9, 2023

adiholden Dec 10, 2023

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

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

Conversation

theyueli commented Dec 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theyueli Dec 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theyueli Dec 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theyueli commented Dec 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theyueli commented Dec 8, 2023 •

edited

theyueli Dec 8, 2023 •

edited

theyueli Dec 8, 2023 •

edited