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

[improve][test] Add integration test for entry filters #17396

Merged
merged 4 commits into from
Sep 16, 2022

Conversation

andrasbeni
Copy link
Contributor

@andrasbeni andrasbeni commented Sep 1, 2022

Master Issue: #17132

Motivation

Add tests to prevent unintended changes to how the broker uses Entry Filters.

Modifications

  • Created an entry filter, PatternEntryFilter.
  • Created a test that configures the entry filter for the brokers and verifies that the entry filter functions correctly

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

Added integration tests for entry filters

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • doc-not-needed
    This change only affects tests

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 1, 2022
@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

1 similar comment
@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM, left a trivial comment.

String messageValue = producer.getProducerName() + "-" + i;
MessageId messageId = producer.newMessage()
.value(messageValue)
.property("filter_property", messageValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can use the static param FILTER_PROPERTY of the class PatternEntryFilter.java as the key here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it would be cleaner, but I don't think this alone is a strong enough reason to introduce a dependency between the two maven modules these classes are in, so I decided to duplicate this string instead. Let me know what you think.

@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun pulsar-ci-checks-completed

@andrasbeni
Copy link
Contributor Author

/pulsarbot rerun pulsar-ci-checks-completed

@andrasbeni
Copy link
Contributor Author

/pulsarbot run-failure-checks

@andrasbeni
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui merged commit 4ba219d into apache:master Sep 16, 2022
codertmy pushed a commit to codertmy/pulsar that referenced this pull request Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants