-
Notifications
You must be signed in to change notification settings - Fork 179
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
Properly batch Client.delete_many() calls #184
Conversation
This one is going to require more time to review than I have this morning, so I'll try to take a closer look this weekend. |
No rush. Thanks for being willing to consider it. |
I looked at this and it looks OK, but I'll let @cgordon take a look and validate. Once he's happy with it, we can merge and cut a release. Thanks for the change |
if not self.sock: | ||
self._connect() | ||
|
||
try: | ||
self.sock.sendall(cmd) | ||
self.sock.sendall(b''.join(cmds)) |
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.
I have a slight preference for joining here with the \r\n
instead of adding it to each cmd
, but it's not a huge deal.
@@ -648,13 +650,13 @@ def version(self): | |||
A string of the memcached version. | |||
""" | |||
cmd = b"version\r\n" | |||
result = self._misc_cmd(cmd, b'version', False) | |||
results = self._misc_cmd([cmd], b'version', False) | |||
before, _, after = results[0].partition(b' ') |
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.
Thanks for the improved readability!
Looks great, apologies for the delay, and thank you! |
Essentially the same change as #182 but for
delete_many()
and its underlying_misc_cmd()
method. This includes benchmarks for the delete methods.Benchmarks for the current codebase with irrelevant lines trimmed:
With this diff applied,
delete_multi()
is significantly improved: