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

Implement delete multiple keys #74

Closed

Conversation

dusansimic
Copy link
Contributor

Passing array of keys to .delete() function deletes multiple keys all at once.

Closes: #41

Passing array of keys to `.delete()` function deletes multiple keys all
at once.
@dusansimic
Copy link
Contributor Author

If this looks good, I'll just add it to docs and it's ready to pull.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6112654 on dusansimic:implement-delete-multiple-keys into 05a8b07 on lukechilds:master.

Copy link
Contributor

@lukechilds lukechilds left a comment

Choose a reason for hiding this comment

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

While you could implement this by looping over an array inside Keyv and deleting each value individually, that could result in pretty poor performance.

We could maybe add a canBatchDelete boolean property to some storage adapters which we can check for in Keyv.

If it's true, we pass it an array of keys and the storage adapter will handle the array and delete all keys in a single performant atomic operation.

If it's false/undefined, we fall back to just looping over the values and deleting each individually.

This would enable support for all storage adapters in a backwards compatible way, while giving performance speedups to the storage adapters that support bulk deletes.

@@ -90,7 +93,12 @@ class Keyv extends EventEmitter {
key = this._getKeyPrefix(key);
const store = this.opts.store;
return Promise.resolve()
.then(() => store.delete(key));
.then(() => {
if (store instanceof Map && Array.isArray(key)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't only be implemented for Maps, the main benefit of using Keyv over alternatives is that it provides a consistent interface across multiple backends.

If we implement a feature only for a single backend it would break that.

@@ -75,6 +75,21 @@ test.serial('.set(key, val, ttl) where ttl is "0" overwrites default tll option
tk.reset();
});

test.serial('.delete(key) where key is a single key', async t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests should be in @keyv/test-suite so they cover all storage adapters, not just Keyv + Map.

@jaredwray
Copy link
Owner

@dusansimic - wanted to check in on this as there were some items to resolve. Any status on this? If not, going to close this pull request out and not merge. Let me know how I can help.

@dusansimic
Copy link
Contributor Author

@jaredwray sadly, no status on this. I worked on this a really long time ago and haven't had time to make any significant progress. I think it's better to close this down since I don't have much free time now either. Sorry for keeping it open without any news for a long time.

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.

Delete multiple keys in a single operation
4 participants