Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: provide basic logging to catch possible command errors #3213

Merged
merged 2 commits into from
Jun 26, 2024
Merged

chore: provide basic logging to catch possible command errors #3213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b0a8de
chore: provide basic logging to catch possible command errors
romange Jun 17, 2024
3e65d44
chore: comments
romange Jun 25, 2024
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
12 changes: 12 additions & 0 deletions src/facade/conn_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,16 @@ size_t ConnectionContext::UsedMemory() const {
return dfly::HeapSize(rbuilder_) + dfly::HeapSize(authed_username) + dfly::HeapSize(acl_commands);
}

void ConnectionContext::SendError(std::string_view str, std::string_view type) {
rbuilder_->SendError(str, type);
}

void ConnectionContext::SendError(ErrorReply error) {
rbuilder_->SendError(error);
}

void ConnectionContext::SendError(OpStatus status) {
rbuilder_->SendError(status);
}

} // namespace facade
12 changes: 3 additions & 9 deletions src/facade/conn_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,11 @@ class ConnectionContext {
return res;
}

void SendError(std::string_view str, std::string_view type = std::string_view{}) {
rbuilder_->SendError(str, type);
}
virtual void SendError(std::string_view str, std::string_view type = std::string_view{});
Copy link
Contributor

Choose a reason for hiding this comment

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

The only problem I see here is that these can be skipped if the caller uses reply_builder()->SendError which we do spuriously around the codebase. From my understanding we want this to cover lua scripts so it's worth double checking that we actually use this interface rather than the indirect way reply_builder()->SendError() which bypasses this flow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, fixed.


void SendError(ErrorReply error) {
rbuilder_->SendError(error);
}
virtual void SendError(ErrorReply error);

void SendError(OpStatus status) {
rbuilder_->SendError(status);
}
virtual void SendError(OpStatus status);

void SendStored() {
rbuilder_->SendStored();
Expand Down
2 changes: 1 addition & 1 deletion src/facade/ok_main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class OkService : public ServiceInterface {

void DispatchMC(const MemcacheParser::Command& cmd, std::string_view value,
ConnectionContext* cntx) final {
cntx->reply_builder()->SendError("");
cntx->SendError("");
}

ConnectionContext* CreateContext(util::FiberSocketBase* peer, Connection* owner) final {
Expand Down
6 changes: 3 additions & 3 deletions src/server/cluster/cluster_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,12 +585,12 @@ void ClusterFamily::DflyClusterGetSlotInfo(CmdArgList args, ConnectionContext* c
do {
auto sid = parser.Next<uint32_t>();
if (sid > kMaxSlotNum)
return rb->SendError("Invalid slot id");
return cntx->SendError("Invalid slot id");
slots_stats.emplace_back(sid, SlotStats{});
} while (parser.HasNext());

if (auto err = parser.Error(); err)
return rb->SendError(err->MakeReply());
return cntx->SendError(err->MakeReply());

fb2::Mutex mu;

Expand Down Expand Up @@ -675,7 +675,7 @@ void ClusterFamily::DflySlotMigrationStatus(CmdArgList args, ConnectionContext*
if (parser.HasNext()) {
node_id = parser.Next<std::string_view>();
if (auto err = parser.Error(); err) {
return rb->SendError(err->MakeReply());
return cntx->SendError(err->MakeReply());
}
}

Expand Down
23 changes: 23 additions & 0 deletions src/server/conn_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,29 @@ size_t ConnectionContext::UsedMemory() const {
return facade::ConnectionContext::UsedMemory() + dfly::HeapSize(conn_state);
}

void ConnectionContext::SendError(std::string_view str, std::string_view type) {
string_view name = cid ? cid->name() : string_view{};

VLOG(1) << "Sending error " << str << " " << type << " during " << name;
facade::ConnectionContext::SendError(str, type);
}

void ConnectionContext::SendError(facade::ErrorReply error) {
string_view name = cid ? cid->name() : string_view{};

VLOG(1) << "Sending error " << error.ToSv() << " during " << name;
facade::ConnectionContext::SendError(std::move(error));
}

void ConnectionContext::SendError(facade::OpStatus status) {
if (status != facade::OpStatus::OK) {
string_view name = cid ? cid->name() : string_view{};
VLOG(1) << "Sending error " << status << " during " << name;
}

facade::ConnectionContext::SendError(status);
}

void ConnectionState::ExecInfo::Clear() {
DCHECK(!preborrowed_interpreter); // Must have been released properly
state = EXEC_INACTIVE;
Expand Down
4 changes: 4 additions & 0 deletions src/server/conn_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,10 @@ class ConnectionContext : public facade::ConnectionContext {

size_t UsedMemory() const override;

void SendError(std::string_view str, std::string_view type = std::string_view{}) override;
void SendError(facade::ErrorReply error) override;
void SendError(facade::OpStatus status) override;

// Whether this connection is a connection from a replica to its master.
// This flag is true only on replica side, where we need to setup a special ConnectionContext
// instance that helps applying commands coming from master.
Expand Down
4 changes: 2 additions & 2 deletions src/server/debugcmd.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,10 +520,10 @@ void DebugCmd::Replica(CmdArgList args) {
}
return;
} else {
return rb->SendError("I am master");
return cntx_->SendError("I am master");
}
}
return rb->SendError(UnknownSubCmd("replica", "DEBUG"));
return cntx_->SendError(UnknownSubCmd("replica", "DEBUG"));
}

void DebugCmd::Load(string_view filename) {
Expand Down
22 changes: 11 additions & 11 deletions src/server/dflycmd.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,15 +184,15 @@ void DflyCmd::Thread(CmdArgList args, ConnectionContext* cntx) {
string_view arg = ArgS(args, 1);
unsigned num_thread;
if (!absl::SimpleAtoi(arg, &num_thread)) {
return rb->SendError(kSyntaxErr);
return cntx->SendError(kSyntaxErr);
}

if (num_thread < pool->size()) {
if (int(num_thread) != ProactorBase::me()->GetPoolIndex()) {
if (!cntx->conn()->Migrate(pool->at(num_thread))) {
// Listener::PreShutdown() triggered
if (cntx->conn()->socket()->IsOpen()) {
return rb->SendError(kInvalidState);
return cntx->SendError(kInvalidState);
}
return;
}
Expand All @@ -201,7 +201,7 @@ void DflyCmd::Thread(CmdArgList args, ConnectionContext* cntx) {
return rb->SendOk();
}

return rb->SendError(kInvalidIntErr);
return cntx->SendError(kInvalidIntErr);
}

void DflyCmd::Flow(CmdArgList args, ConnectionContext* cntx) {
Expand All @@ -214,20 +214,20 @@ void DflyCmd::Flow(CmdArgList args, ConnectionContext* cntx) {
if (args.size() == 5) {
seqid.emplace();
if (!absl::SimpleAtoi(ArgS(args, 4), &seqid.value())) {
return rb->SendError(facade::kInvalidIntErr);
return cntx->SendError(facade::kInvalidIntErr);
}
}

VLOG(1) << "Got DFLY FLOW master_id: " << master_id << " sync_id: " << sync_id_str
<< " flow: " << flow_id_str << " seq: " << seqid.value_or(-1);

if (master_id != sf_->master_replid()) {
return rb->SendError(kBadMasterId);
return cntx->SendError(kBadMasterId);
}

unsigned flow_id;
if (!absl::SimpleAtoi(flow_id_str, &flow_id) || flow_id >= shard_set->size()) {
return rb->SendError(facade::kInvalidIntErr);
return cntx->SendError(facade::kInvalidIntErr);
}

auto [sync_id, replica_ptr] = GetReplicaInfoOrReply(sync_id_str, rb);
Expand All @@ -236,7 +236,7 @@ void DflyCmd::Flow(CmdArgList args, ConnectionContext* cntx) {

unique_lock lk(replica_ptr->mu);
if (replica_ptr->replica_state != SyncState::PREPARATION)
return rb->SendError(kInvalidState);
return cntx->SendError(kInvalidState);

// Set meta info on connection.
cntx->conn()->SetName(absl::StrCat("repl_flow_", sync_id));
Expand All @@ -254,7 +254,7 @@ void DflyCmd::Flow(CmdArgList args, ConnectionContext* cntx) {
if (!cntx->conn()->Migrate(shard_set->pool()->at(flow_id))) {
// Listener::PreShutdown() triggered
if (cntx->conn()->socket()->IsOpen()) {
return rb->SendError(kInvalidState);
return cntx->SendError(kInvalidState);
}
return;
}
Expand Down Expand Up @@ -311,7 +311,7 @@ void DflyCmd::Sync(CmdArgList args, ConnectionContext* cntx) {

// TODO: Send better error
if (*status != OpStatus::OK)
return rb->SendError(kInvalidState);
return cntx->SendError(kInvalidState);
}

LOG(INFO) << "Started sync with replica " << replica_ptr->address << ":"
Expand Down Expand Up @@ -349,7 +349,7 @@ void DflyCmd::StartStable(CmdArgList args, ConnectionContext* cntx) {
shard_set->RunBlockingInParallel(std::move(cb));

if (*status != OpStatus::OK)
return rb->SendError(kInvalidState);
return cntx->SendError(kInvalidState);
}

LOG(INFO) << "Transitioned into stable sync with replica " << replica_ptr->address << ":"
Expand Down Expand Up @@ -436,7 +436,7 @@ void DflyCmd::TakeOver(CmdArgList args, ConnectionContext* cntx) {

if (*status != OpStatus::OK) {
sf_->service().SwitchState(GlobalState::TAKEN_OVER, GlobalState::ACTIVE);
return rb->SendError("Takeover failed!");
return cntx->SendError("Takeover failed!");
}
cntx->SendOk();

Expand Down
10 changes: 5 additions & 5 deletions src/server/hset_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ void HGetGeneric(CmdArgList args, ConnectionContext* cntx, uint8_t getall_mask)
rb->SendStringArr(absl::Span<const string>{*result},
is_map ? RedisReplyBuilder::MAP : RedisReplyBuilder::ARRAY);
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand Down Expand Up @@ -822,7 +822,7 @@ void HSetFamily::HMGet(CmdArgList args, ConnectionContext* cntx) {
rb->SendNull();
}
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand All @@ -842,7 +842,7 @@ void HSetFamily::HGet(CmdArgList args, ConnectionContext* cntx) {
if (result.status() == OpStatus::KEY_NOTFOUND) {
rb->SendNull();
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}
}
Expand Down Expand Up @@ -965,7 +965,7 @@ void HSetFamily::HScan(CmdArgList args, ConnectionContext* cntx) {
rb->SendBulkString(k);
}
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand Down Expand Up @@ -1149,7 +1149,7 @@ void HSetFamily::HRandField(CmdArgList args, ConnectionContext* cntx) {
else
rb->SendEmptyArray();
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand Down
16 changes: 8 additions & 8 deletions src/server/json_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1439,7 +1439,7 @@ void JsonFamily::Set(CmdArgList args, ConnectionContext* cntx) {
rb->SendNull();
}
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand Down Expand Up @@ -1510,7 +1510,7 @@ void JsonFamily::Resp(CmdArgList args, ConnectionContext* cntx) {
SendJsonValue(rb, it);
}
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand Down Expand Up @@ -1793,7 +1793,7 @@ void JsonFamily::ArrPop(CmdArgList args, ConnectionContext* cntx) {
}
}
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand Down Expand Up @@ -1865,7 +1865,7 @@ void JsonFamily::ObjKeys(CmdArgList args, ConnectionContext* cntx) {
}
}
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand Down Expand Up @@ -1916,7 +1916,7 @@ void JsonFamily::NumIncrBy(CmdArgList args, ConnectionContext* cntx) {
if (result) {
rb->SendBulkString(*result);
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand Down Expand Up @@ -1944,7 +1944,7 @@ void JsonFamily::NumMultBy(CmdArgList args, ConnectionContext* cntx) {
if (result) {
rb->SendBulkString(*result);
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}

Expand Down Expand Up @@ -1992,7 +1992,7 @@ void JsonFamily::Type(CmdArgList args, ConnectionContext* cntx) {
if (result.status() == OpStatus::KEY_NOTFOUND) {
rb->SendNullArray();
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}
}
Expand Down Expand Up @@ -2112,7 +2112,7 @@ void JsonFamily::Get(CmdArgList args, ConnectionContext* cntx) {
if (result == facade::OpStatus::KEY_NOTFOUND) {
rb->SendNull(); // Match Redis
} else {
rb->SendError(result.status());
cntx->SendError(result.status());
}
}
}
Expand Down
14 changes: 7 additions & 7 deletions src/server/list_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,7 +735,7 @@ void MoveGeneric(ConnectionContext* cntx, string_view src, string_view dest, Lis
break;

default:
rb->SendError(result.status());
cntx->SendError(result.status());
break;
}
}
Expand Down Expand Up @@ -776,7 +776,7 @@ void BRPopLPush(CmdArgList args, ConnectionContext* cntx) {
break;

default:
return rb->SendError(op_res.status());
return cntx->SendError(op_res.status());
break;
}
}
Expand Down Expand Up @@ -819,7 +819,7 @@ void BLMove(CmdArgList args, ConnectionContext* cntx) {
break;

default:
return rb->SendError(op_res.status());
return cntx->SendError(op_res.status());
break;
}
}
Expand Down Expand Up @@ -1034,7 +1034,7 @@ void ListFamily::LIndex(CmdArgList args, ConnectionContext* cntx) {
if (result) {
rb->SendBulkString(result.value());
} else if (result.status() == OpStatus::WRONG_TYPE) {
rb->SendError(result.status());
cntx->SendError(result.status());
} else {
rb->SendNull();
}
Expand Down Expand Up @@ -1217,13 +1217,13 @@ void ListFamily::BPopGeneric(ListDir dir, CmdArgList args, ConnectionContext* cn

switch (popped_key.status()) {
case OpStatus::WRONG_TYPE:
return rb->SendError(kWrongTypeErr);
return cntx->SendError(kWrongTypeErr);
case OpStatus::CANCELLED:
case OpStatus::TIMED_OUT:
return rb->SendNullArray();
case OpStatus::KEY_MOVED:
// TODO: proper error for moved
return rb->SendError("-MOVED");
return cntx->SendError("-MOVED");
default:
LOG(ERROR) << "Unexpected error " << popped_key.status();
}
Expand Down Expand Up @@ -1277,7 +1277,7 @@ void ListFamily::PopGeneric(ListDir dir, CmdArgList args, ConnectionContext* cnt
case OpStatus::KEY_NOTFOUND:
return rb->SendNull();
case OpStatus::WRONG_TYPE:
return rb->SendError(kWrongTypeErr);
return cntx->SendError(kWrongTypeErr);
default:;
}

Expand Down
Loading
Loading