Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/server/conn_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,12 @@ bool ConnectionState::ClientTracking::ShouldTrackKeys() const {
return false;
}

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

Choose a reason for hiding this comment

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

so by default we always track keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes when tracking is on

return true;
}

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

} // namespace dfly
21 changes: 17 additions & 4 deletions src/server/conn_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,15 @@ struct ConnectionState {
// If seq_num == caching_seq_num + 1 then we know that we should Track().
class ClientTracking {
public:
enum Options : uint8_t {
NONE, // NO subcommand, that is no OPTIN and no OUTPUT was used when CLIENT TRACKING was
// called. We track all keys of read commands.
OPTIN, // OPTIN was used with CLIENT TRACKING. We only track keys of read commands preceded
// by CACHING TRUE command.
OPTOUT // OPTOUT was used with CLIENT TRACKING. We track all keys of read commands except the
// ones preceded by a CACHING FALSE command.
};

// Sets to true when CLIENT TRACKING is ON
void SetClientTracking(bool is_on) {
tracking_enabled_ = is_on;
Expand All @@ -196,9 +205,9 @@ struct ConnectionState {
++seq_num_;
}

// Set if OPTIN subcommand is used in CLIENT TRACKING
void SetOptin(bool optin) {
optin_ = optin;
// Set if OPTIN/OPTOUT subcommand is used in CLIENT TRACKING
void SetOption(Options option) {
option_ = option;
}

// Check if the keys should be tracked. Result adheres to the state machine described above.
Expand All @@ -219,10 +228,14 @@ struct ConnectionState {
caching_seq_num_ = 0;
}

bool HasOption(Options option) const {
return option_ == option;
}

private:
// a flag indicating whether the client has turned on client tracking.
bool tracking_enabled_ = false;
bool optin_ = false;
Options option_ = NONE;
// sequence number
size_t seq_num_ = 0;
size_t caching_seq_num_ = 0;
Expand Down
23 changes: 18 additions & 5 deletions src/server/server_family.cc
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,8 @@ void ClientTracking(CmdArgList args, ConnectionContext* cntx) {
return cntx->SendError(kSyntaxErr);

bool is_on = false;
bool optin = false;
using Tracking = ConnectionState::ClientTracking;
Tracking::Options option = Tracking::NONE;
if (parser.Check("ON").IgnoreCase()) {
is_on = true;
} else if (!parser.Check("OFF").IgnoreCase()) {
Expand All @@ -475,7 +476,9 @@ void ClientTracking(CmdArgList args, ConnectionContext* cntx) {

if (parser.HasNext()) {
if (parser.Check("OPTIN").IgnoreCase()) {
optin = true;
option = Tracking::OPTIN;
} else if (parser.Check("OPTOUT").IgnoreCase()) {
option = Tracking::OPTOUT;
} else {
return cntx->SendError(kSyntaxErr);
}
Expand All @@ -485,7 +488,7 @@ void ClientTracking(CmdArgList args, ConnectionContext* cntx) {
++cntx->subscriptions;
}
cntx->conn_state.tracking_info_.SetClientTracking(is_on);
cntx->conn_state.tracking_info_.SetOptin(optin);
cntx->conn_state.tracking_info_.SetOption(option);
return cntx->SendOk();
}

Expand All @@ -499,16 +502,26 @@ void ClientCaching(CmdArgList args, ConnectionContext* cntx) {
return cntx->SendError(kSyntaxErr);
}

using Tracking = ConnectionState::ClientTracking;
CmdArgParser parser{args};
if (parser.Check("YES").IgnoreCase()) {
bool is_multi = cntx->transaction && cntx->transaction->IsMulti();
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");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compatibility! We previously allowed this!

}
} else if (parser.Check("NO").IgnoreCase()) {
if (!cntx->conn_state.tracking_info_.HasOption(Tracking::OPTOUT)) {
return cntx->SendError(
"ERR CLIENT CACHING NO is only valid when tracking is enabled in OPTOUT mode");
}
cntx->conn_state.tracking_info_.ResetCachingSequenceNumber();
} else {
return cntx->SendError(kSyntaxErr);
}

bool is_multi = cntx->transaction && cntx->transaction->IsMulti();
cntx->conn_state.tracking_info_.SetCachingSequenceNumber(is_multi);

cntx->SendOk();
}

Expand Down
55 changes: 53 additions & 2 deletions src/server/server_family_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,14 @@ TEST_F(ServerFamilyTest, ClientTrackingOnAndOff) {
resp = Run({"CLIENT", "TRACKING", "ON"});
EXPECT_THAT(resp.GetString(), "OK");

resp = Run({"CLIENT", "CACHING", "YES"});
EXPECT_THAT(
resp, ErrArg("ERR CLIENT CACHING YES is only valid when tracking is enabled in OPTIN mode"));

resp = Run({"CLIENT", "CACHING", "NO"});
EXPECT_THAT(
resp, ErrArg("ERR CLIENT CACHING NO is only valid when tracking is enabled in OPTOUT mode"));

// case 3. turn off client tracking
resp = Run({"CLIENT", "TRACKING", "OFF"});
EXPECT_THAT(resp.GetString(), "OK");
Expand Down Expand Up @@ -347,7 +355,6 @@ TEST_F(ServerFamilyTest, ClientTrackingMultiOptin) {
Run({"SET", "TMP", "10"});
Run({"CLIENT", "CACHING", "YES"});
Run({"GET", "FOO"});
Run({"CLIENT", "CACHING", "NO"});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a bug! It should have been rejected!

Run({"GET", "BAR"});
Run({"EXEC"});

Expand All @@ -357,7 +364,51 @@ TEST_F(ServerFamilyTest, ClientTrackingMultiOptin) {
EXPECT_EQ(InvalidationMessagesLen("IO0"), 5);
Run({"SET", "BAR", "10"});
Run({"GET", "BAR"});
EXPECT_EQ(InvalidationMessagesLen("IO0"), 5);
EXPECT_EQ(InvalidationMessagesLen("IO0"), 6);
}

TEST_F(ServerFamilyTest, ClientTrackingOptout) {
Run({"HELLO", "3"});
// Check stickiness
Run({"CLIENT", "TRACKING", "ON", "OPTOUT"});
Run({"GET", "FOO"});
Run({"SET", "FOO", "BAR"});
Run({"GET", "BAR"});
Run({"SET", "BAR", "FOO"});
EXPECT_EQ(InvalidationMessagesLen("IO0"), 2);

// Switch off tracking for a single command
Run({"CLIENT", "CACHING", "NO"});
Run({"GET", "FOO"});
Run({"SET", "FOO", "BAR"});
EXPECT_EQ(InvalidationMessagesLen("IO0"), 2);
}

TEST_F(ServerFamilyTest, ClientTrackingMultiOptout) {
Run({"HELLO", "3"});
// Check stickiness
Run({"CLIENT", "TRACKING", "ON", "OPTOUT"});

Run({"MULTI"});
Run({"GET", "FOO"});
Run({"SET", "TMP", "10"});
Run({"GET", "FOOBAR"});
Run({"EXEC"});

Run({"SET", "FOO", "10"});
Run({"SET", "FOOBAR", "10"});
EXPECT_EQ(InvalidationMessagesLen("IO0"), 2);

// CACHING enclosed in MULTI
Run({"MULTI"});
Run({"CLIENT", "CACHING", "NO"});
Run({"GET", "TMP"});
Run({"GET", "TMP_TMP"});
Run({"SET", "TMP", "10"});
Run({"SET", "TMP_TMP", "10"});
Run({"EXEC"});

EXPECT_EQ(InvalidationMessagesLen("IO0"), 2);
}

TEST_F(ServerFamilyTest, ClientTrackingUpdateKey) {
Expand Down