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: adds new filters functions #1281

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

TheGhoul21
Copy link

No description provided.

@TheGhoul21
Copy link
Author

#1280
I think that actually everything could be brought to a unique implementation where we fill OrderedConfig with the correct additional key (namely filter_subject and filter_subjects).

Since is my very first commit I wasn't sure about that.

@Jarema
Copy link
Member

Jarema commented Jun 20, 2024

Yes, this is correct. We were discussing having separate methods only because of compatibility with server version.

Instead we landed on this:
https://github.com/nats-io/nats-architecture-and-design/blob/main/adr/ADR-8.md?plain=1#L139

// KeysWithFilters returns a filtered list of keys in the bucket.
// Historically this method returned a complete slice of all keys in the bucket,
// however clients should return interable result.
// Languages can implement the list of filters in most idiomatic way - as an iterator, variadic argument, slice, etc.
// When multiple filters are passed, client library should check `consumer info` from `consumer create method` if the filters are matching,
// as nats-server < 2.10 would ignore them.
KeysWithFilters(filter []string) ([]string, error)

@TheGhoul21
Copy link
Author

ok, I changed the code. I hope the way I instantiate the config is fine.
Tests are passing, I'm just not sure if it's the most idiomatic way to implement this.

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.

Hey!
Thank you for your contribution!

This looks pretty good.
Few comments made.

async-nats/tests/kv_tests.rs Outdated Show resolved Hide resolved
@@ -795,6 +796,28 @@ mod kv {
keys.sort();
assert_eq!(vec!["bar", "foo"], keys);


let mut keys_with_filter = kv
Copy link
Member

Choose a reason for hiding this comment

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

I would add few more keys, and try a bit more complex filters and check them.

Copy link
Author

Choose a reason for hiding this comment

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

I added more tests following the docs here
I think filters in the format foo.b* are not supported, as * can only replace whole words, am I right?

async-nats/src/jetstream/kv/mod.rs Outdated Show resolved Hide resolved
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.

Functionally looks good, but one improvement requested.

async-nats/src/jetstream/kv/mod.rs Outdated Show resolved Hide resolved
@Jarema
Copy link
Member

Jarema commented Jul 29, 2024

@TheGhoul21 are you interested in finishing this PR? 🙂

@TheGhoul21
Copy link
Author

I pushed the changes you suggested.
The feature check fails when server_2_10 is not set. How should I handle that?

@Jarema
Copy link
Member

Jarema commented Jul 30, 2024

@TheGhoul21 You need to put the function behind the feature flag

    #[cfg(feature = "server_2_10")]

@TheGhoul21
Copy link
Author

keys() calls the same function for backward support, maybe something like this?

match (filters.next(), filters.next()) {
            (Some(first), None) => {
                config.filter_subject = first;
            }
            (Some(first), Some(second)) => {
                #[cfg(feature = "server_2_10")]
                {
                    config.filter_subjects = vec![first, second];
                    config.filter_subjects.extend(filters);
                }
                #[cfg(not(feature = "server_2_10"))]
                {
                    config.filter_subject = first;
                    // maybe a warning
                }
            }
            _ => {}
        }

I hate the duplicate code though

@Jarema
Copy link
Member

Jarema commented Jul 30, 2024

Ah, right.

@TheGhoul21 The sole reason for utilising the feature flag is to avoid such situations where not all passed filters are properly set.

I'm afraid that duplication is the way to go here.
We can, however, after 2.11 is released, revisit this, as 2.10 will be the last supported version.

@TheGhoul21 TheGhoul21 requested a review from Jarema July 30, 2024 11:27
#[cfg(not(feature = "server_2_10"))]
{
config.filter_subject = first;
// maybe a warning
Copy link
Member

Choose a reason for hiding this comment

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

This is not the way to go.

As said before - copy-paste is the way to go, for now.

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.

2 participants