-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[PIP-105] Part-2 Support pluggable entry filter in Dispatcher #12970
Conversation
conf/broker.conf
Outdated
# Class name of Pluggable entry filter that can decide whether the entry needs to be filtered | ||
# You can use this class to decide which entries can be sent to consumers. | ||
# Multiple classes need to be separated by commas. | ||
entryFilterClassNames= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a directory to maintain all the filters? otherwise, we need to copy the filter jar to the lib dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another point is we should load the class by a separate classloader that extends from the pulsar classloader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@315157973 I see you've added annotations in API files, except that, will you need descriptions (what the feature is
and how to use this feature
) to docs? Somewhere like here?
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/EntryFilter.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/EntryFilter.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/EntryFilter.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/EntryFilter.java
Outdated
Show resolved
Hide resolved
…ntryFilter.java Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
…ntryFilter.java Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
…ntryFilter.java Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
…ntryFilter.java Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work.
Please take a look to my comments
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/EntryFilter.java
Outdated
Show resolved
Hide resolved
private SubscriptionOption subscriptionOption; | ||
private MessageMetadata msgMetadata; | ||
|
||
public void reset() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is only for the implementation, do not keep it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Context does not need to be made into an interface, just make it into a class directly
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/EntryFilter.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/EntryFilter.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/EntryFilter.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/plugin/EntryFilterForTest2.java
Outdated
Show resolved
Hide resolved
Hello, I have processed as required, please take a look again @eolivelli |
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterProvider.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/FilterContext.java
Outdated
Show resolved
Hide resolved
@codelipenghui @eolivelli PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did my last pass.
I am sorry I missed some comments on my first pass.
...broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterWithClassLoader.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterProvider.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilter.java
Show resolved
Hide resolved
@eolivelli All done, thanks |
@eolivelli PTAL |
ping @eolivelli PTAL thanks |
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/plugin/EntryFilterProvider.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/plugin/EntryFilterTest.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/plugin/EntryFilterTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM great work !
* up/master: (75 commits) [website][upgrade]feat: website upgrade / docs migration - 2.5.1 Get Started/Concepts and Architecture/Pulsar Schema (apache#13030) Fix environment variable assignment in startup scripts (apache#13025) update 2.8.x (apache#13029) [Doc] add tips for Pulsar tools (apache#13044) Suggest to use tlsPort instead of deprecated TlsEnable (apache#13039) Integration tests for function-worker rebalance and drain operations. (apache#13058) fix(functions): missing runtime set in GoInstanceConfig (apache#13031) [pulsar-admin] Add get-replicated-subscription-status command for topic (apache#12891) [Broker] Consider topics in pulsar/system namespace as system topics (apache#13050) Fix typo: correct sizeUint to sizeUnit (apache#13040) fix-12894 (apache#12896) Don't attempt to delete pending ack store unless transactions are enabled (apache#13041) [Perf] Evaluate the current protocol version once (apache#13045) Fix Issue apache#12885, Unordered consuming case in Key_Shared subscription (apache#12890) [broker]Optimize topicMaxMessageSize with topic local cache. (apache#12830) [PIP-105] Part-2 Support pluggable entry filter in Dispatcher (apache#12970) [website] Modify admin-api-topic.md document (apache#12996) add missed import (apache#13037) [metadata] Add RocksdbMetadataStore (apache#12776) [C] Add pulsar_client_subscribe_multi_topics and pulsar_client_subscribe_pattern (apache#12965) ... # Conflicts: # site2/website-next/docusaurus.config.js # site2/website-next/versioned_sidebars/version-2.6.1-sidebars.json # site2/website-next/versions.json
Motivation
see #12269
Modifications
Add pluggable entry filter
Documentation
doc-required