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: get back on the decision to put a hard limit on command interface #4203

Merged
merged 2 commits into from
Nov 27, 2024
Merged

chore: get back on the decision to put a hard limit on command interface #4203

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
fc2bf4c
chore: get back on the decision to put a hard limit on command interface
romange Nov 27, 2024
ecaba1c
chore: fix commentS
romange Nov 27, 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
10 changes: 9 additions & 1 deletion src/server/command_registry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,19 @@ optional<facade::ErrorReply> CommandId::Validate(CmdArgList tail_args) const {

CommandId&& CommandId::SetHandler(Handler2 f) && {
handler_ = [f = std::move(f)](CmdArgList args, Transaction* tx, facade::SinkReplyBuilder* builder,
facade::ConnectionContext*) { f(args, tx, builder); };
ConnectionContext*) { f(args, tx, builder); };

return std::move(*this);
}

CommandId&& CommandId::SetHandler(Handler3 f) && {
handler_ = [f = std::move(f)](CmdArgList args, Transaction* tx, facade::SinkReplyBuilder* builder,
ConnectionContext* cntx) {
f(std::move(args), CommandContext{tx, builder, cntx});
};
return std::move(*this);
};

CommandRegistry::CommandRegistry() {
vector<string> rename_command = GetFlag(FLAGS_rename_command);

Expand Down
14 changes: 14 additions & 0 deletions src/server/command_registry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ static_assert(!IsEvalKind(""));
// Per thread vector of command stats. Each entry is {cmd_calls, cmd_latency_agg in usec}.
using CmdCallStats = std::pair<uint64_t, uint64_t>;

struct CommandContext {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a thought: since this will be used quite a lot, maybe rename it to CmdContext or CmdCntx?
Feel free to say no obviously, I just don't think it makes the code less readable.

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 strange thing for me is that we don't have the class Command, but have CommandContext. From my point of view, the best naming for a context should look like Command::Context or Cmd::Cntx. The same for other contexts they are not self-sufficient so I would like to move them into other classes or they should lost the context part of the name

CommandContext(Transaction* _tx, facade::SinkReplyBuilder* _rb, ConnectionContext* cntx)
: tx(_tx), rb(_rb), conn_cntx(cntx) {
}

Transaction* tx;
facade::SinkReplyBuilder* rb;
ConnectionContext* conn_cntx;
};

class CommandId : public facade::CommandId {
public:
// NOTICE: name must be a literal string, otherwise metrics break! (see cmd_stats_map in
Expand All @@ -95,6 +105,8 @@ class CommandId : public facade::CommandId {
fu2::function_base<true, true, fu2::capacity_default, false, false,
void(CmdArgList, Transaction*, facade::SinkReplyBuilder*) const>;

using Handler3 = fu2::function_base<true, true, fu2::capacity_default, false, false,
void(CmdArgList, const CommandContext&) const>;
using ArgValidator = fu2::function_base<true, true, fu2::capacity_default, false, false,
std::optional<facade::ErrorReply>(CmdArgList) const>;

Expand Down Expand Up @@ -130,6 +142,8 @@ class CommandId : public facade::CommandId {

CommandId&& SetHandler(Handler2 f) &&;

CommandId&& SetHandler(Handler3 f) &&;

CommandId&& SetValidator(ArgValidator f) && {
validator_ = std::move(f);
return std::move(*this);
Expand Down
Loading
Loading