Enable multiple event listeners to consume for Polaris events#3973
Enable multiple event listeners to consume for Polaris events#3973nandorKollar wants to merge 14 commits intoapache:mainfrom
Conversation
|
@jbonofre @adutra @adnanhemani would you mind share your thoughts about this approach? |
d0242ed to
09c5600
Compare
09c5600 to
aad3caf
Compare
adutra
left a comment
There was a problem hiding this comment.
@nandorKollar this is look promising thanks!
I think we should avoid calling dispatch() altogether (and avoid materializing the PolarisEvent instance on the heap) if we know the event type is not needed. I would like us to consider that, if a user deactivates all listeners, they would get a performance comparable to what they would get if there were no events in Polaris at all.
...ache/polaris/service/events/jsonEventListener/aws/cloudwatch/AwsCloudWatchEventListener.java
Outdated
Show resolved
Hide resolved
...ache/polaris/service/events/jsonEventListener/aws/cloudwatch/AwsCloudWatchEventListener.java
Outdated
Show resolved
Hide resolved
...rvice/src/main/java/org/apache/polaris/service/events/PolarisEventListenerConfiguration.java
Outdated
Show resolved
Hide resolved
...rvice/src/main/java/org/apache/polaris/service/events/PolarisEventListenerConfiguration.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListeners.java
Outdated
Show resolved
Hide resolved
8b05fcf to
1c71676
Compare
| public void onStartup(@Observes StartupEvent event) { | ||
| String listerTypes = configuration.types().orElse(""); | ||
| if (listerTypes.isBlank()) { | ||
| return; |
There was a problem hiding this comment.
If we don't have types here, it means we never reach type() merging bellow (line 51).
If a user only set polaris.event-listener.type (the old config) without setting types, the onStartup() method just returns directly, and no listener is registered.
It means it's a breaking change.
Imho, the type() merge logic should be before the early return, or this check should check both types and type (to be backward compatible).
There was a problem hiding this comment.
Good point, thanks! This is indeed broken, fixed in 1a0aaa5
| * identifier. | ||
| */ | ||
| String type(); | ||
| Optional<String> type(); |
There was a problem hiding this comment.
We don't need this one anymore, right?
There was a problem hiding this comment.
I think we need it to read the old config: polaris.event-listener.type. It is optional to allow one to omit. The value of this config (if present), and the new one polaris.event-listener.types should be merged. We should also indicate that polaris.event-listener.type is deprecated, should not be used.
There was a problem hiding this comment.
Hmm indeed our evolution guidelines considers changes to the configuration as breaking changes: https://polaris.apache.org/releases/1.3.0/evolution/
So, I think we indeed need to deprecate this config but keep it around for some time.
There was a problem hiding this comment.
How can we mark it deprecated?
There was a problem hiding this comment.
You should:
- Annotate the method with
@Deprecated - Add a note to the changelog.
There was a problem hiding this comment.
Although RelocateConfigSourceInterceptor sounds really promising, I'm afraid it won't work for us. It relocates the property, but we need to merge the values: if both old and new property is set, the values should be merged. We can alternatively say, that the new property overrides the old one, if both are set:
Option 1:
polaris.event-listener.type=aws-cloudwatch
polaris.event-listener.types=persistence-in-memory-buffer
result is: persistence-in-memory-buffer, aws-cloudwatch
Option 2:
polaris.event-listener.type=aws-cloudwatch
polaris.event-listener.types=persistence-in-memory-buffer
result is persistence-in-memory-buffer, because the new property overrides the old one
The second approach is super simple with FallbackConfigSourceInterceptor, the first one needs a bit more work. Due to simplicity I'd vote for option 2, if both are set (why would someone set both btw? maybe we can emit a warning if both are set btw, or when the old property is still used) What do you think?
There was a problem hiding this comment.
Option 2 LGTM. My rationale: if the admin user sets the new property, we can assume the user is aware of the old config deprecation and took care to set the new value correctly.
There was a problem hiding this comment.
Re: warning - IMHO it's not worth the effort (same rationale applies), but if it's simple to implement, why not?
There was a problem hiding this comment.
It looks like Option 2 got implemented already 👍
The warning is optional from my POV.
There was a problem hiding this comment.
I'm also ok with Option 2. Given that type will be deprecated with this PR, it doesn't make much sense to keep respecting it if types is already defined.
| * Comma separated list of event listers, each item must be a registered {@link | ||
| * PolarisEventListener} identifier. | ||
| */ | ||
| Optional<String> types(); |
There was a problem hiding this comment.
| Optional<String> types(); | |
| Optional<List<String>> types(); |
It's a bit odd to see an Optional<List<...> – but that's how SmallRye Config wants it 😅
There was a problem hiding this comment.
Update: Optional<Set<String>> types(); is probably better.
48b5fbd to
c9f843b
Compare
|
|
||
| polaris.file-io.type=default | ||
|
|
||
| polaris.event-listener.type=no-op |
There was a problem hiding this comment.
As there's no need to set this property to no-op, should we also delete NoOpPolarisEventListener? It will be a breaking change, because anyone who still refers to no-op with the old polaris.event-listener.type (or even with the new polaris.event-listener.types) will hit the following error: No bean found for required type [interface org.apache.polaris.service.events.listeners.PolarisEventListener] and qualifiers [[@jakarta.enterprise.inject.Any(), @io.smallrye.common.annotation.Identifier(value="no-op")]]
We can mark it deprecated though.
There was a problem hiding this comment.
Yes, I think we could delete the no-op listener. Regarding breaking changes, our evolution guidelines accept that: https://polaris.apache.org/releases/1.3.0/evolution/
…tomatically by smallrye
ee21f22 to
716f6db
Compare
Thanks, this makes sense, however I've further questions related to this. Do you happen to know why does AwsCloudWatchEventListener filter only for a single event type? I think event listeners should accept any event type, and allow users to configure a particular event listener to listen for only a certain events, but the listener code itself should not assume that it is working only with event type xyz. |
Completely agree here. Listeners should NOT filter events, and especially not in a hard-coded fashion – that's a user decision. @adnanhemani may know why the CoudWatch listener was designed that way. Imho it's not very useful – it can only be useful if you are interested in after-refresh-table events, and hence, in certain table optimization use cases. |
|
Oh and we shouldn't forget #2573 either. We need to find a way to trigger event cleanup, or even set retention period for events persisted in the database. |
I'm all in for breaking the work in many smaller PRs, but I'm a little confused: you said you would like to introduce the event listener configuration in a follow-up PR – but you also wrote you would like to make each event listener configurable in this PR. Could you clarify this small item? |
I recommend to target only focus on making multiple event listeners work in this PR, and rather implement the rest in a second PR. The only configuration parameter for now is deprecating |
That works for me. I think this PR is good to go then, but it would still be nice to add a note to the changelog about the deprecation of |
Should we add it only to the changelog? EDIT: never mind, found the way to deprecate it in the doc too. :) |
dimas-b
left a comment
There was a problem hiding this comment.
Sorry about last minute comments. The PR LGTM overall. My only suggestion is to try and add automatic redirects for the old config name.
runtime/service/src/main/java/org/apache/polaris/service/events/EventBusConfigurer.java
Outdated
Show resolved
Hide resolved
| * identifier. | ||
| */ | ||
| String type(); | ||
| Optional<String> type(); |
There was a problem hiding this comment.
We could probably add a config redirect too.
Cf.
...ervice/src/main/java/org/apache/polaris/service/events/PolarisServiceBusEventDispatcher.java
Outdated
Show resolved
Hide resolved
...ice/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java
Outdated
Show resolved
Hide resolved
...estFixtures/java/org/apache/polaris/service/events/listeners/TestPolarisEventDispatcher.java
Outdated
Show resolved
Hide resolved
16170de to
b570a2e
Compare
…er_service_bus # Conflicts: # runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java
|
|
||
| @ApplicationScoped | ||
| @DefaultBean | ||
| public class PolarisServiceBusEventDispatcher implements PolarisEventDispatcher { |
There was a problem hiding this comment.
Can we add a Java doc for this class?
CHANGELOG.md
Outdated
|
|
||
| - The configuration option `polaris.rate-limiter.token-bucket.window` is no longer supported and should be removed. | ||
| - `PolarisConfigurationStore` has been deprecated for removal. | ||
| - The configuration option `polaris.event-listener.type` is no longer supported and should be removed. Please use `polaris.event-listener.types` instead. |
There was a problem hiding this comment.
It mighty be OK to make this breaking change, given event listener are not widely used ATM. Could we share it in the mailing list to gather more feedback to double check? cc @adnanhemani .
If it turns out to be fine, we should call it out here that it's a breaking change.
There was a problem hiding this comment.
NVM. It seems to be deprecated. I think the word here isn't accurate.
| - The configuration option `polaris.event-listener.type` is no longer supported and should be removed. Please use `polaris.event-listener.types` instead. | |
| - The configuration option `polaris.event-listener.type` is deprecated and will be removed later. Please use `polaris.event-listener.types` instead. |
| import io.vertx.core.eventbus.EventBus; | ||
| import jakarta.enterprise.event.Observes; | ||
|
|
||
| public class EventBusConfigurer { |
There was a problem hiding this comment.
Do we need @ApplicationScoped for this class?
There was a problem hiding this comment.
IDK off the top of my head 😅 ... but it does not hurt, I'm pretty sure.
There was a problem hiding this comment.
ServiceProducers have the maybeBootstrap() observer without @ApplicationScoped, so I'd guess it is NOT required here.
There was a problem hiding this comment.
If we drop @ApplicationScoped it might be worth trying to make the observer method static.
There was a problem hiding this comment.
This is not a blocker from my POV
flyrain
left a comment
There was a problem hiding this comment.
Thanks @nandorKollar for working on it. Can we add test to configure polaris.event-listener.types with 2+ listeners (e.g., 'test,in-memory-buffer') and verify all receive events? Can we add test to verify deprecated polaris.event-listener.type still works?
|
nit: I updated the PR title (typo on lister instead of listener 😄 ). |
dimas-b
left a comment
There was a problem hiding this comment.
LGTM, pending resolution of open comment threads 👍 Thanks, @nandorKollar !
|
I will look into this later today, please hold merging until then. |
adnanhemani
left a comment
There was a problem hiding this comment.
Looks almost there, just a few questions on my end - and then one comment from @flyrain about adding a Javadoc. Will approve it after those are done!
|
|
||
| @ApplicationScoped | ||
| public class EventBusConfigurer { | ||
| private static final String LOCAL_CODEC_NAME = "local"; |
There was a problem hiding this comment.
I'm not an expert but after searching around, it seems like this value is supposed to be unique per codec. So shouldn't we name this something like "polaris-events-local" or something to ensure there is no potential future clash naming-wise?
| */ | ||
| public class OAuthClientCredentialsParametersDpo extends AuthenticationParametersDpo { | ||
|
|
||
| private static final Joiner COLON_JOINER = Joiner.on(":"); |
| package org.apache.polaris.extension.auth.opa.test; | ||
|
|
||
| import io.quarkus.test.junit.QuarkusTestProfile; | ||
| import io.quarkus.test.junit.QuarkusTestProfile.TestResourceEntry; |
This PR enables multiple event listeners watching for Polaris events.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)