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.

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: add Rename Command #2455

Merged
merged 1 commit into from
Mar 6, 2024
Merged

feat: add Rename Command #2455

merged 1 commit into from
Mar 6, 2024

Conversation

Mixficsol
Copy link
Collaborator

@Mixficsol Mixficsol commented Mar 6, 2024

fix: #2452

@github-actions github-actions bot added the ✏️ Feature New feature or request label Mar 6, 2024
@Mixficsol Mixficsol added the 3.5.3 label Mar 6, 2024
conf/pika.conf Outdated Show resolved Hide resolved
@chejinge
Copy link
Collaborator

chejinge commented Mar 6, 2024

看下这个命令是否可以加到config set config。write行列中,比如中途要修改或者什么的

@Mixficsol
Copy link
Collaborator Author

看下这个命令是否可以加到config set config。write行列中,比如中途要修改或者什么的

这个应该是一开始就定好的,和之前userblacklist一样,需要禁掉的命令应该是一开始就被禁掉的,中途不应该临时又决定给用户用这些禁掉的命令

@chejinge
Copy link
Collaborator

chejinge commented Mar 6, 2024

看下这个命令是否可以加到config set config。write行列中,比如中途要修改或者什么的

这个应该是一开始就定好的,和之前userblacklist一样,需要禁掉的命令应该是一开始就被禁掉的,中途不应该临时又决定给用户用这些禁掉的命令

ok 建议把 userblacklist讨论好怎么做 没有相互影响在合并

@@ -50,6 +50,14 @@ void PikaCmdTableManager::InitCmdTable(void) {
}
}

void PikaCmdTableManager::RenameCommand(const std::string before, const std::string after) {
auto it = cmds_->find(before);
if (it != cmds_->end()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

考虑 after 为 "" 的情况,比如在 conf 文件里面配置的 hget:
rename-command : hget
rename-command : set 360set

redis 里面会先把命令删除了:https://github.com/redis/redis/blob/unstable/src/config.c
image

另外,如果 before cmd 不存在,直接返回错误,让用户去修改配置文件

Copy link
Collaborator

Choose a reason for hiding this comment

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

可以参考redis的逻辑,尽量保持一致

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -50,7 +50,7 @@ class BaseConf {
bool GetConfIntHuman(const std::string& name, int* value) const;
bool GetConfInt64(const std::string& name, int64_t* value) const;
bool GetConfInt64Human(const std::string& name, int64_t* value) const;

bool GetConfStr(const std::string& name, std::vector<std::string>* values) const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里直接使用 GetConfStrMulti 方法,直接获取一个数组,不用新加方法

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -178,6 +178,19 @@ bool BaseConf::GetConfStr(const std::string& name, std::string* val) const {
return false;
}

bool BaseConf::GetConfStr(const std::string& name, std::vector<std::string>* values) const {
Copy link
Collaborator

@luky116 luky116 Mar 6, 2024

Choose a reason for hiding this comment

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

这个方法用 GetConfStrMulti 替换,不用新加

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/pika_conf.cc Outdated
@@ -465,7 +467,23 @@ int PikaConf::Load() {
GetConfStrMulti("user", &users_);

GetConfStr("aclfile", &aclFile_);

GetConfStrMulti("rename-command", &cmds_);
std::string before, after;
Copy link
Collaborator

@luky116 luky116 Mar 6, 2024

Choose a reason for hiding this comment

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

你测试下这么配置:
rename-command : GET GET22
rename-command : HSET

解析 HSET 的时候,这个 after 值是 GET22,保留了上一次的值,期望是将 HSET 命令删除

Copy link
Collaborator Author

@Mixficsol Mixficsol Mar 6, 2024

Choose a reason for hiding this comment

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

HSET还是会删除的,因为逻辑是只要这个before有值且是Pika的命令,首先这个命令是一定会被删除的,至于有没有替换那得看替换的值是否是空值

conf/pika.conf Show resolved Hide resolved
@luky116 luky116 merged commit a753d90 into OpenAtomFoundation:unstable Mar 6, 2024
12 checks passed
chengyu-l pushed a commit to chengyu-l/pika that referenced this pull request Mar 7, 2024
@Mixficsol Mixficsol deleted the RenameCommand branch March 14, 2024 12:15
bigdaronlee163 pushed a commit to bigdaronlee163/pika that referenced this pull request Jun 8, 2024
cheniujh pushed a commit to cheniujh/pika that referenced this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.5.3 ✏️ Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Rename Command
3 participants