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(kvstore): delete all keys #1181

Merged
merged 10 commits into from
Apr 18, 2024
Merged

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Apr 16, 2024

This PR fixes the performance issues with deleting all keys from a KV Store and requires https://github.com/fastly/go-fastly/releases/tag/v9.3.1

This PR also adds support for configuring the deletion of keys via the fastly kv-store delete --all command (as customers didn't expect to find that behaviour inside of fastly kv-store-entry delete --all)

@Integralist Integralist added the bug Something isn't working label Apr 16, 2024
@Integralist Integralist force-pushed the integralist/kv-store-delete-feat branch from 72a4f00 to 4abb01a Compare April 16, 2024 16:42
@Integralist Integralist force-pushed the integralist/kv-store-delete-feat branch from 4abb01a to 31e8983 Compare April 16, 2024 16:45
Copy link
Contributor

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

I don't see anything obviously wrong with this code, so my suggestion is to profile it and see what's taking the most time. Maybe there's something blocking that is hard to spot when reading but will be obvious in a profile.

pkg/commands/kvstore/delete.go Show resolved Hide resolved
@Integralist Integralist marked this pull request as ready for review April 17, 2024 10:08
@Integralist Integralist force-pushed the integralist/kv-store-delete-feat branch from 1be80cc to 5074e14 Compare April 17, 2024 10:26
@Integralist
Copy link
Collaborator Author

@cee-dub this is ready for a re-review. Thanks

@Integralist Integralist force-pushed the integralist/kv-store-delete-feat branch from e069510 to 551bbd0 Compare April 17, 2024 16:21
@Integralist Integralist force-pushed the integralist/kv-store-delete-feat branch from a10b10a to acdece5 Compare April 17, 2024 16:37
Copy link
Contributor

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

For 10k keys, the delete operation is now faster than bulk load. Bravo!

pkg/commands/kvstoreentry/delete.go Outdated Show resolved Hide resolved
pkg/commands/kvstoreentry/delete.go Show resolved Hide resolved
@dennismartensson
Copy link
Contributor

I am not sure this is deciereble. We have made the decision to not allow wholesale deletion of of stores. This feels like a bit of a work around to that decision and is probably worst then what we could do if we allow wholesale deletes.

@cee-dub
Copy link
Contributor

cee-dub commented Apr 17, 2024

@dennismartensson I'm aware of some discussions about not allowing wholesale deletes. There are customer requests for this feature to exist, e.g. for CI environments. Do you have any ideas how to address those customer requests without this? I agree this solution is a workaround, but that's the result of the decision not to allow wholesale deletion of KV stores.

@dennismartensson
Copy link
Contributor

I am just worried that a developer fat fingers a delete of a store with millions of objects or TB of data and we have no way of recovering that for them it is just gone and might take them a very long time to recover.

@Integralist
Copy link
Collaborator Author

I am just worried that a developer fat fingers a delete of a store with millions of objects

Just as a side note this 'delete all keys' feature has existed in the CLI for a while now (~10 months without issue).

We already present the following warning prompt to the user before the deletion process is started...

Screenshot 2024-04-18 at 09 27 00

And in this PR I've added a similar warning for the fastly kv-store delete command...

Screenshot 2024-04-18 at 09 28 20

@dennismartensson
Copy link
Contributor

dennismartensson commented Apr 18, 2024 via email

@Integralist Integralist merged commit fbd3794 into main Apr 18, 2024
8 checks passed
@Integralist Integralist deleted the integralist/kv-store-delete-feat branch April 18, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants