-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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
feat(server): Add support for aliasing commands #4782
base: main
Are you sure you want to change the base?
Conversation
499fe92
to
be23a1a
Compare
The helper will be used to parse renamed commands as well as aliases. Signed-off-by: Abhijat Malviya <[email protected]>
A command can be aliased by supplying the flag: --command_alias=__PING=PING to the server startup. A map of alias is retained. When finding a command id, first we look up into the original command map, then if not found there we lookup through the command alias map. This results in two find operations for an aliased command. Signed-off-by: Abhijat Malviya <[email protected]>
be23a1a
to
dd40902
Compare
Signed-off-by: Abhijat Malviya <[email protected]>
dd40902
to
cf015a7
Compare
PR can be reviewed now but will wait for merge for 1.29 |
Mostly LGTM -- minor comments. For me I need to validate aliases and ping tomorrow. |
Signed-off-by: Abhijat Malviya <[email protected]>
Signed-off-by: Abhijat Malviya <[email protected]>
The goal of this task is to be able to filter commands from dashboard. |
This is missing in this PR right now. Is there a list of metrics that are used to count the commands? I will make changes to update them for the alias. |
@adiholden after giving this more thought I suggest that this PR should be merged as it is (once reviewed and approved etc), it is self contained and adds the aliasing feature. I can do a follow up PR to change metrics to be counted per alias or command. I see the metrics with command name labels are
Are there any other metrics to be updated? |
Sure, I did not review the PR. I want you to think on the next step of the implementation and see that this PR will support this change need for metrics.
I believe that is all. Also not sure what is the expected behaviour for info commandstats, maybe @romange can answer this |
Make sure you follow up on this and you know how you are going to implement the next step so this PR will supprot the change needed for the metrics |
Signed-off-by: Abhijat Malviya <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM minor comments
src/server/dragonfly_test.cc
Outdated
@@ -2,29 +2,29 @@ | |||
// See LICENSE for licensing terms. | |||
// | |||
|
|||
#include "absl/strings/str_split.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "absl/strings/str_split.h" | |
#include <absl/strings/str_split.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
|
||
if (const auto it = cmd_aliases_.find(cmd); it != cmd_aliases_.end()) { | ||
if (const auto alias_lookup = cmd_map_.find(it->second); alias_lookup != cmd_map_.end()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we expect that it->second
will exist in the cmd_map_
? In other words shouldn't this if be a DCHECK + a find
. The registry is not mutable and the alias map is initialized during bootstrap so I would expect that this alias_lookup == cmd_map_.end()
can't be true right ?
Basically what I am trying to say is that the alias map should never point to a command that does not exist in the cmd_map
correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The aliases are processed before commands are registered, in the ctor of command registry. It wouldn't be possible to validate if the alias is correct at that time.
Signed-off-by: Abhijat Malviya <[email protected]>
Support for aliasing a command is added with the command line flag
command_alias
. Multiple aliases of the same command are allowed.Example run:
server
client
commands which have been renamed using
rename_command
should be aliased with the renamed command name instead of original.TODO: update docs