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 filtering to KV method returning all keys #797

Merged
merged 14 commits into from
Sep 11, 2024

Conversation

saurabhojha
Copy link
Contributor

@saurabhojha saurabhojha commented Sep 4, 2024

Resolves: #766

This PR introduces the kvStore_KeysWithFilters function to the kvStore API. This function allows users to retrieve keys from a key-value store bucket that match one or more subject-like filters. The function helps in querying specific keys based on patterns, enhancing the flexibility and usability of key retrieval.

@saurabhojha saurabhojha changed the title Add implementation for filtering keys from kv store Add filtering to KV method returning all keys Sep 4, 2024
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. I made some comments. Lots of them are just code style, but the most important is to not duplicate the code for the main function.

src/nats.h Outdated Show resolved Hide resolved
src/nats.h Outdated Show resolved Hide resolved
src/nats.h Outdated Show resolved Hide resolved
src/nats.h Outdated Show resolved Hide resolved
src/kv.c Outdated Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Some more modifications needed.

src/nats.h Outdated Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Outdated Show resolved Hide resolved
test/test.c Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Before I continue the review, please make sure that your changes in the core Keys() function are needed. I am not sure they are.

src/kv.c Outdated Show resolved Hide resolved
src/kv.c Outdated Show resolved Hide resolved
src/kv.c Outdated Show resolved Hide resolved
src/kv.c Outdated Show resolved Hide resolved
src/kv.c Outdated Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

By the way, you may want to combine all subsequent changes in a single commit before pushing it (to reduce CI runs).

test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
test/test.c Show resolved Hide resolved
@saurabhojha saurabhojha force-pushed the saurabh/c/nats/kv-filters branch from 79266ca to 1f62ad0 Compare September 4, 2024 19:31
@saurabhojha saurabhojha force-pushed the saurabh/c/nats/kv-filters branch from 1f62ad0 to e212786 Compare September 4, 2024 19:41
@saurabhojha saurabhojha requested a review from kozlovic September 4, 2024 19:42
@saurabhojha
Copy link
Contributor Author

Hi @kozlovic , made the changes you requested. Please let me know if anything else has to be changed.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Let's move the options around.

But, this brings the question: @levb, had you consider having the filter options as part of the kvWatchOptions instead of adding a kvStore_WatchMulti()? In PR #750, I don't see me suggesting that, but since this has never been released yet, it would be possible to change it. If that had been the case, there would not even be a need for a new kvStore_KeysWithFilters() API, since kvStore_Keys() accepts a kvWatchOptions, we could have passed the filters through that (we would have needed to change a bit to use filters if specified and call Watch() - or if documenting that Watch() uses filters over provided key, no change at all).

src/kv.c Outdated Show resolved Hide resolved
src/kv.c Outdated Show resolved Hide resolved
src/kv.c Outdated Show resolved Hide resolved
src/nats.h Outdated Show resolved Hide resolved
@kozlovic
Copy link
Member

kozlovic commented Sep 5, 2024

@saurabhojha @levb My comment about passing filters through options may be a moot point if the ADR asks libraries to add a new API.

@saurabhojha saurabhojha requested a review from kozlovic September 5, 2024 15:51
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM! But I will let @levb decide if we merge as-is or if he wants to revisit the WatchMulti or not. If we do merge this PR, I would recommend the "squash on merge", since there are a lot of commits on that one.

@levb
Copy link
Collaborator

levb commented Sep 5, 2024

@saurabhojha @kozlovic I've been quite sick the last couple days, I'll review this soon. @saurabhojha Thank you for the contribution!

@kozlovic
Copy link
Member

kozlovic commented Sep 5, 2024

@levb So sorry to hear that. Please rest, there is really no urgency in reviewing this PR.

@saurabhojha
Copy link
Contributor Author

@levb take care and please rest. this can be reviewed later as well.

@levb
Copy link
Collaborator

levb commented Sep 9, 2024

Let me confirm with @Jarema, I like the simplification of using kvWatchOptions.

@levb
Copy link
Collaborator

levb commented Sep 11, 2024

After discussing with @Jarema 1/5 we should mimic the Go API, and have these as separate methods. In a future release we'd attempt to simplify the multiple subjects/filters interface.

@levb levb merged commit 5dfb172 into nats-io:main Sep 11, 2024
20 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 11, 2024
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.

Add filtering to KV method returning all keys
3 participants