feat(kv): implement KeysWithFilters and ListKeysWithFilters with tests #1711
Conversation
- Added `KeysWithFilters` to filter and return a list of matching keys. - Added `ListKeysWithFilters` to filter keys and return them as an iterable lister. - Included test cases for both methods to validate filtering logic and key retrieval.
There was a problem hiding this comment.
Hello @somratdutta, thank you for the contribution and sorry it took so long to review this. There are a few issues with this PR and it'll be easier to list them in a single comment rather than commenting on individual lines:
-
The filtering should not be done client-side, for a few reasons. Firstly, it is not efficient as you're fetching all keys from the server, even if you're interested in 1% of them based on the provided filter. Secondly, the filter you apply will not work with wildcards, which users should be able to provide. For filtering, we should leverage the
FilterSubjectsfield onConsumerConfigand let the server do the work and simply return results to the user.
That presents another issue unfortunately. While you can add a single filter toWatch()and use it to filter the keys (instead ofWatchAll()), there currently is no version ofWatch()which accepts multiple filters. -
We agreed internally that we don't really want to bloat the API with too many new methods, so we settled on adding a single one:
ListKeysFiltered(ctx context.Context, filters ...string) (KeyLister, error). This will allow us to add 0 or more keys easily and iterate over results in efficient way (not having to allocate memory for all keys at once if there are many of them). Also in this new methodWatchOptcan be dropped as it does not really contribute to the result.
I understand that this was not properly outlined in the related issue, apologies for that. Here's my proposal - I'll work on adding a watcher which can accept multiple filters (it should not take longer than 1-2 days I hope) and after it's done you'll be able to rework this PR to use this new API - obviously if you're still willing to.
|
Thanks, @piotrpio for the detailed feedback on the PR. |
|
Hello @somratdutta! We've merged the PR for |
|
@somratdutta could you let us know if you'll be working on that? If you don't have time for that, I can continue your work :) |
|
@piotrpio Thanks for the ping, I was busy with the |
- Replaced custom filtering logic with the new `WatchFiltered()` method. - Updated associated tests in `kv_test.go` to validate the new implementation with various filter patterns.
|
@piotrpio I have added the changes, let me know if this is fine. |
…ed test (#2) * refactor(kv): utilize WatchFiltered() for key filtering logic - Replaced custom filtering logic with the new `WatchFiltered()` method. - Updated associated tests in `kv_test.go` to validate the new implementation with various filter patterns. * work on comments
|
Hi @piotrpio. |
|
@somratdutta we will get back to this with @piotrpio next week. |
KeysWithFiltersto filter and return a list of matching keys.ListKeysWithFiltersto filter keys and return them as an iterable lister.Resolves: #1655