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

Fix rename-command didn't work when using it multiple times #1047

Merged
merged 11 commits into from
Oct 28, 2022

Conversation

tanruixiang
Copy link
Member

This close #1040

src/config/config.cc Outdated Show resolved Hide resolved
src/config/config.cc Outdated Show resolved Hide resolved
src/config/config.cc Show resolved Hide resolved
src/config/config.cc Outdated Show resolved Hide resolved
@PragmaTwice
Copy link
Member

PragmaTwice commented Oct 27, 2022

https://github.com/apache/incubator-kvrocks/blob/debd8988d6f6978593ad35b7e06d0c3e773ee825/src/config/config.cc#L767-L771

Since ALL MultiStringField cannot be rewritten, so I think you need to change it to something like

if (iter->second.IsMulti())

in case of someone use MultiStringField but forget to include the config key in this check.

@git-hulk
Copy link
Member

https://github.com/apache/incubator-kvrocks/blob/debd8988d6f6978593ad35b7e06d0c3e773ee825/src/config/config.cc#L767-L771

Since ALL MultiStringField cannot be rewritten, so I think you need to change it to something like

if (iter->second.IsMulti())

in case of someone use MultiStringField but forget to include the config key in this check.

Yes, that's right.

@PragmaTwice PragmaTwice requested a review from git-hulk October 28, 2022 06:48
@git-hulk
Copy link
Member

Thanks @tanruixiang @PragmaTwice, merging...

@git-hulk git-hulk merged commit 29d5a95 into apache:unstable Oct 28, 2022
@tanruixiang tanruixiang deleted the fix_rename_bug branch October 28, 2022 12:29
@git-hulk git-hulk changed the title fix: supports multiple rename-commands Fix rename-command didn't work when using it multiple times Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rename commands don't work
3 participants