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 and renamenx #299

Merged
merged 3 commits into from
May 20, 2024

Conversation

zztaki
Copy link
Contributor

@zztaki zztaki commented Apr 30, 2024

#30
This PR aims to add rename and renamenx commands for pikiwidb.

Running ./pikiwidbtests.sh basic
image

There are a few places that I think need to be focused on.

@github-actions github-actions bot added the ✏️ Feature New feature or request label Apr 30, 2024
@zztaki zztaki force-pushed the feat/add-rename branch from b064415 to 0308afb Compare May 3, 2024 09:56
@@ -148,6 +148,18 @@ class Redis {
virtual Status ZsetsTTL(const Slice& key, uint64_t* timestamp);
virtual Status SetsTTL(const Slice& key, uint64_t* timestamp);

virtual Status StringsRename(const Slice& key, Redis* new_inst, const Slice& newkey);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

key and newkey may not be on the same shard. So I use new_inst to represent newkey's shard and pass it as an arg.

} else if (s.IsNotFound()) {
client->SetRes(CmdRes::kNotFound, s.ToString());
} else if (s.IsCorruption()) {
client->AppendInteger(0); // newkey already exists
Copy link
Contributor Author

@zztaki zztaki May 3, 2024

Choose a reason for hiding this comment

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

I'm not sure if there is a better error code to represent that newkey already exists, thus returning (int) 0.

@dingxiaoshuai123 dingxiaoshuai123 requested a review from Mixficsol May 6, 2024 09:39
@dingxiaoshuai123
Copy link
Collaborator

May I ask if it is possible to supplement some Go tests to validate the functionality of this command?

@zztaki
Copy link
Contributor Author

zztaki commented May 6, 2024

May I ask if it is possible to supplement some Go tests to validate the functionality of this command?

I haven't implemented it yet. Are there any good examples? :)

@dingxiaoshuai123
Copy link
Collaborator

May I ask if it is possible to supplement some Go tests to validate the functionality of this command?

I haven't implemented it yet. Are there any good examples? :)

May I ask if it is possible to supplement some Go tests to validate the functionality of this command?

I haven't implemented it yet. Are there any good examples? :)

There are Go test files under the path pikiwidb/tests, which can be referred to for inspiration.

@dingxiaoshuai123
Copy link
Collaborator

May I ask if it is possible to supplement some Go tests to validate the functionality of this command?

I haven't implemented it yet. Are there any good examples? :)

https://github.com/redis/go-redis/blob/master/commands_test.go This might be helpful to you.

@zztaki
Copy link
Contributor Author

zztaki commented May 6, 2024

May I ask if it is possible to supplement some Go tests to validate the functionality of this command?

I haven't implemented it yet. Are there any good examples? :)

https://github.com/redis/go-redis/blob/master/commands_test.go This might be helpful to you.

Thank you for your help! I will complete it at night.

@zztaki
Copy link
Contributor Author

zztaki commented May 7, 2024

cicheck

@zztaki
Copy link
Contributor Author

zztaki commented May 7, 2024

image

Current PR is blocked by this error.

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


image

@dingxiaoshuai123
Copy link
Collaborator

image

Current PR is blocked by this error.

Please update the code.

@zztaki
Copy link
Contributor Author

zztaki commented May 8, 2024

image
Current PR is blocked by this error.

Please update the code.

Done

@AlexStocks AlexStocks merged commit cd5e466 into OpenAtomFoundation:unstable May 20, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants