chore: get back on the decision to put a hard limit on command interface #4203
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Limiting commands to only Transaction* and SinkReplyBuilder does not hold. We need sometimes to access context fields for multitude of reasons. But I do not want to pass the huge ConnectionContext object because, it's hard then to track unusual access patterns. The compromise: to introduce CommandContext that currently has tx, rb and extended fields. It will be relatively easy to identify irregular access patterns by tracking the extended field. This commit is the first one in series of probably 10-15 commits. No functional changes here. Signed-off-by: Roman Gershman <[email protected]>
romange
commented
Nov 27, 2024
| @@ -974,14 +974,14 @@ OpStatus SetCmd::CachePrevIfNeeded(const SetCmd::SetParams& params, DbSlice::Ite | |||
| return OpStatus::OK; | |||
| } | |||
|
|
|||
| void StringFamily::Set(CmdArgList args, Transaction* tx, SinkReplyBuilder* builder, | |||
| ConnectionContext* cntx) { | |||
| void StringFamily::Set(CmdArgList args, const CommandContext& cmnd_cntx) { | |||
There was a problem hiding this comment.
all the changes here are search and replace
chakaz
reviewed
Nov 27, 2024
chakaz
approved these changes
Nov 27, 2024
| @@ -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 { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Limiting commands to only Transaction* and SinkReplyBuilder does not hold. We need sometimes to access context fields for multitude of reasons.
But I do not want to pass the huge ConnectionContext object because, it's hard then to track unusual access patterns.
The compromise: to introduce CommandContext that currently has tx, rb and extended fields. It will be relatively easy to identify irregular access patterns by tracking the extended field.
This commit is the first one in series of probably 10-15 commits. No functional changes here.