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

Add optional parameter to LPOP/RPOP to pop more than 1 element #594

Merged
merged 3 commits into from
May 25, 2022

Conversation

torwig
Copy link
Contributor

@torwig torwig commented May 19, 2022

Related TCL-test were copied from the Redis repository.
Method List::Pop refactored: output param std::string *elem became the last one for convenience purposes.

Please let me know if something should be checked additionally.

@torwig torwig force-pushed the add_count_lpop_rpop branch from 24fceff to 6b6cb6e Compare May 19, 2022 11:07
@git-hulk
Copy link
Member

git-hulk commented May 24, 2022

Thanks @torwig, we can use popMulti to implement the pop as well? since I think pop is the special condition for popMulti.

@git-hulk git-hulk requested review from git-hulk and ShooterIT May 24, 2022 14:39
@torwig
Copy link
Contributor Author

torwig commented May 24, 2022

@git-hulk I had a similar idea, but then I read the Redis docs:

When called without the count argument:
Bulk string reply: the value of the first element, or nil ($-1) when key does not exist.
When called with the count argument:
Array reply: list of popped elements, or nil (*-1) when key does not exist.

and I saw that BLPOP/BRPOP and RPopLPush use just Pop without count option, so I decided to leave Pop for popping a single element and introduce PopMulti for popping more than one element.
Additionally, it's more explicit to return multiple elements from PopMulti inside a vector than to return just a single element inside a vector (if it's a special case for pop).

@git-hulk
Copy link
Member

@torwig Oooop, my bad that didn't have a check at where's using pop. Current implementation is good to me.

git-hulk
git-hulk previously approved these changes May 24, 2022
@ShooterIT
Copy link
Member

thanks @torwig Whatever i don't think it is a good idea to have PopMulti and Pop, we should implement Pop as a special PopMulti

@torwig
Copy link
Contributor Author

torwig commented May 25, 2022

@ShooterIT I replaced Pop implementation with multi-pop's one and pushed.
Or do you think I should implement Pop like this:

rocksdb::Status List::Pop(const Slice &user_key, bool left, std::string *elem) {
  std::vector<std::string> elems;
  auto s = PopMulti(user_key, left, 1, &elems);
  if (!s.ok()) return s;
  *elem = elems[0];
  return rocksdb::Status::OK();
}

?
And in this case, the code will be much cleaner.

@ShooterIT
Copy link
Member

rocksdb::Status List::Pop(const Slice &user_key, bool left, std::string *elem) {
  std::vector<std::string> elems;
  auto s = PopMulti(user_key, left, 1, &elems);
  if (!s.ok()) return s;
  *elem = elems[0];
  return rocksdb::Status::OK();
}

I think this idea is better. WDYT @git-hulk

@torwig torwig force-pushed the add_count_lpop_rpop branch from 48ca0d9 to 1fee3b3 Compare May 25, 2022 07:34
@torwig
Copy link
Contributor Author

torwig commented May 25, 2022

@git-hulk I've just pushed the version with the idea that I mentioned (Pop as PopMulti).
Here you can see the previous version with just a single PopMulti method in all the places: torwig@48ca0d9

@git-hulk
Copy link
Member

cool, thanks @torwig

@git-hulk
Copy link
Member

rocksdb::Status List::Pop(const Slice &user_key, bool left, std::string *elem) {
  std::vector<std::string> elems;
  auto s = PopMulti(user_key, left, 1, &elems);
  if (!s.ok()) return s;
  *elem = elems[0];
  return rocksdb::Status::OK();
}

I think this idea is better. WDYT @git-hulk

Yes, I think that retrieves multi elements in pop is a bit weird, it would be better to use popMulti to implement pop.

@git-hulk
Copy link
Member

@ShooterIT Can you help to merge this PR?

@ShooterIT ShooterIT merged commit 834a164 into apache:unstable May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants