feat: client tracking optout #3158

kostasrim · 2024-06-10T15:52:05Z

implement client tracking optout
fix a bug of allowing client caching no without tracking optout
fix error message compatibility with redis/valkey

Partially resolves #3157

kostasrim · 2024-06-10T15:52:40Z

src/server/server_family.cc

-    cntx->conn_state.tracking_info_.SetCachingSequenceNumber(is_multi);
+    if (!cntx->conn_state.tracking_info_.HasOption(Tracking::OPTIN)) {
+      return cntx->SendError(
+          "ERR CLIENT CACHING YES is only valid when tracking is enabled in OPTIN mode");


Compatibility! We previously allowed this!

kostasrim · 2024-06-10T15:52:58Z

src/server/server_family_test.cc

@@ -347,7 +355,6 @@ TEST_F(ServerFamilyTest, ClientTrackingMultiOptin) {
  Run({"SET", "TMP", "10"});
  Run({"CLIENT", "CACHING", "YES"});
  Run({"GET", "FOO"});
-  Run({"CLIENT", "CACHING", "NO"});


That was a bug! It should have been rejected!

kostasrim · 2024-06-10T15:53:37Z

@romange I would like to also add NOLOOP in another PR so plz don't kick any docker build unless I get that as well.

romange · 2024-06-10T16:22:22Z

src/server/conn_context.h

@@ -186,6 +186,8 @@ struct ConnectionState {
  // If seq_num == caching_seq_num + 1 then we know that we should Track().
  class ClientTracking {
   public:
+    enum Options : uint8_t { NONE, OPTIN, OPTOUT };


can you please add the meaning of each option?

romange · 2024-06-10T16:24:50Z

src/server/conn_context.cc

@@ -270,7 +270,17 @@ bool ConnectionState::ClientTracking::ShouldTrackKeys() const {
    return false;
  }

-  return !optin_ || (seq_num_ == (1 + caching_seq_num_));
+  if (option_ == NONE) {


so by default we always track keys?

yes when tracking is on

romange · 2024-06-10T16:29:34Z

src/server/conn_context.cc

+    return true;
+  }
+
+  if (option_ == OPTIN) {


Suggested change

if (option_ == OPTIN) {

bool match = (seq_num_ == (1 + caching_seq_num_));

return option_ == OPTIN ? match : !match;

rwrz · 2024-07-05T17:44:52Z

This is not working on v1.19.2 .. is it?

romange · 2024-07-06T10:54:57Z

@rwrz no, we will release 1.20 soon.

romange · 2024-07-06T10:56:07Z

@rwrz what client library/framework do you use?

rwrz · 2024-07-08T18:42:30Z

I'm using Rueidis plus its locker (like your blog)

kostasrim · 2024-07-09T12:48:15Z

I'm using Rueidis plus its locker (like your blog)

@rwrz we release our new version which includes what you need :)

kostasrim requested review from romange and dranikpg June 10, 2024 15:52

kostasrim commented Jun 10, 2024

View reviewed changes

feat: client tracking optout

551e101

kostasrim force-pushed the client_tracking_optout branch from 310d3b0 to 551e101 Compare June 10, 2024 15:54

kostasrim self-assigned this Jun 10, 2024

romange reviewed Jun 10, 2024

View reviewed changes

address gh comments

ee6bca9

dranikpg approved these changes Jun 11, 2024

View reviewed changes

kostasrim merged commit 2089760 into main Jun 11, 2024
10 checks passed

kostasrim deleted the client_tracking_optout branch June 11, 2024 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: client tracking optout #3158

feat: client tracking optout #3158

kostasrim commented Jun 10, 2024 •

edited

Loading

kostasrim Jun 10, 2024

kostasrim Jun 10, 2024

kostasrim commented Jun 10, 2024

romange Jun 10, 2024

romange Jun 10, 2024

kostasrim Jun 11, 2024

romange Jun 10, 2024

rwrz commented Jul 5, 2024

romange commented Jul 6, 2024

romange commented Jul 6, 2024

rwrz commented Jul 8, 2024

kostasrim commented Jul 9, 2024

	if (option_ == OPTIN) {
	bool match = (seq_num_ == (1 + caching_seq_num_));
	return option_ == OPTIN ? match : !match;

feat: client tracking optout #3158

feat: client tracking optout #3158

Conversation

kostasrim commented Jun 10, 2024 • edited Loading

kostasrim Jun 10, 2024

Choose a reason for hiding this comment

kostasrim Jun 10, 2024

Choose a reason for hiding this comment

kostasrim commented Jun 10, 2024

romange Jun 10, 2024

Choose a reason for hiding this comment

romange Jun 10, 2024

Choose a reason for hiding this comment

kostasrim Jun 11, 2024

Choose a reason for hiding this comment

romange Jun 10, 2024

Choose a reason for hiding this comment

rwrz commented Jul 5, 2024

romange commented Jul 6, 2024

romange commented Jul 6, 2024

rwrz commented Jul 8, 2024

kostasrim commented Jul 9, 2024

kostasrim commented Jun 10, 2024 •

edited

Loading