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(kv): Fixes key listing for async client to match go client #792

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

thomastaylor312
Copy link
Contributor

The go client filters out deleted and purged keys and the async client was not filtering out those keys. This fixes the behavior to be the same and is also slightly more efficient as we aren't iterating over the entire stream of key value history to get the current keys.

I also included an optional commit that I figured I'd do while I was here. The original documentation for this method suggested that the keys method should be returning a Stream rather than an iter. So I implemented a stream version of it if desired. This commit can be popped off (or moved to a separate PR) depending on maintainer preference

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

That is a good direction overall, but needs changes.

I suggest fixing it using current logic with one PR, and refactoring the API as another PR.

async-nats/src/jetstream/kv/mod.rs Outdated Show resolved Hide resolved
@thomastaylor312
Copy link
Contributor Author

Pretty sure the windows test failure is due to a flake, but I can't rerun the tests to check that

The go client [filters out deleted and purged keys](https://github.com/nats-io/nats.go/blob/66009489432be03ff32e5384a2b502fe88f4c635/kv.go#L909)
and the async client was not filtering out those keys. This fixes the
behavior to be the same and is also slightly more efficient as we aren't iterating
over the _entire_ stream of key value history to get the current keys

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
@thomastaylor312
Copy link
Contributor Author

@Jarema friendly reminder ping

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for the contribution!

@Jarema Jarema merged commit a8f934f into nats-io:main Jan 20, 2023
@thomastaylor312 thomastaylor312 deleted the fix/keys_async branch January 20, 2023 21:57
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.

None yet

2 participants