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

Filtering supported #225

Merged
merged 14 commits into from
Aug 23, 2024
Merged

Filtering supported #225

merged 14 commits into from
Aug 23, 2024

Conversation

JiaYingZhang
Copy link
Contributor

Filtering supported

  1. add exchange_command_versions command and check command version;
  2. add version 2 encoder and decoder;
  3. Implemented publish command version2,supporting the filtering feature;
  4. Implemented producer and consumer filtering feature(like java client or python client).

@Gsantomaggio
Copy link
Member

Thank you for the PR

@Gsantomaggio
Copy link
Member

cc @wolf4ood

src/consumer.rs Outdated
impl FilterConfiguration {
pub fn new(
filter_values: Vec<String>,
predicate: impl Fn(&Message) -> bool + 'static + Send + Sync,
Copy link
Contributor

@DanielePalaia DanielePalaia Aug 19, 2024

Choose a reason for hiding this comment

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

Even if normally it is used to avoid "false positives" I think that predicate is Optional and the user can skip it if not necessary @Gsantomaggio to confirm

String::from_utf8(message.data().unwrap().to_vec()).unwrap_or("".to_string())
== "filtering".to_string()
},
false,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you can also add a test for match_unfiltered set to "true"

@DanielePalaia
Copy link
Contributor

Thank you very much for your contribution @JiaYingZhang

It looks good to me. If you can also add an example inside the example folder. Something similar on what you already did in the integration test for producer and consumer

@JiaYingZhang
Copy link
Contributor Author

@DanielePalaia I will add the remaining parts as you suggested.

@JiaYingZhang
Copy link
Contributor Author

@DanielePalaia update for your suggest

  1. consumer predicate type update to Optional and default set None
  2. add filtering example
  3. add consumer test for match_unfiltered set to true

thank you

@DanielePalaia
Copy link
Contributor

DanielePalaia commented Aug 21, 2024

For me it is good to merge. There maybe a few improvements on the example to do but we can do it when we will create the tutorials for this functionality. The action is failing for a rate limit issue on codecov but tests are passing. @wolf4ood for final approval/comments

Copy link
Member

@Gsantomaggio Gsantomaggio left a comment

Choose a reason for hiding this comment

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

Thanks a lot

@Gsantomaggio Gsantomaggio merged commit 49c7103 into rabbitmq:main Aug 23, 2024
1 check failed
@github-actions github-actions bot mentioned this pull request Aug 23, 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.

3 participants