Skip to content

Enable multiple event listeners to consume for Polaris events#3973

Open
nandorKollar wants to merge 14 commits intoapache:mainfrom
nandorKollar:multiple_event_listener_service_bus
Open

Enable multiple event listeners to consume for Polaris events#3973
nandorKollar wants to merge 14 commits intoapache:mainfrom
nandorKollar:multiple_event_listener_service_bus

Conversation

@nandorKollar
Copy link
Contributor

@nandorKollar nandorKollar commented Mar 11, 2026

This PR enables multiple event listeners watching for Polaris events.

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

@nandorKollar
Copy link
Contributor Author

nandorKollar commented Mar 11, 2026

@jbonofre @adutra @adnanhemani would you mind share your thoughts about this approach?

@jbonofre jbonofre self-requested a review March 11, 2026 13:58
@nandorKollar nandorKollar force-pushed the multiple_event_listener_service_bus branch 7 times, most recently from d0242ed to 09c5600 Compare March 11, 2026 19:59
@nandorKollar nandorKollar force-pushed the multiple_event_listener_service_bus branch from 09c5600 to aad3caf Compare March 11, 2026 20:13
@adutra adutra mentioned this pull request Mar 13, 2026
6 tasks
Copy link
Contributor

@adutra adutra left a comment

Choose a reason for hiding this comment

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

@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.

@nandorKollar nandorKollar force-pushed the multiple_event_listener_service_bus branch from 8b05fcf to 1c71676 Compare March 14, 2026 20:20
public void onStartup(@Observes StartupEvent event) {
String listerTypes = configuration.types().orElse("");
if (listerTypes.isBlank()) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks! This is indeed broken, fixed in 1a0aaa5

* identifier.
*/
String type();
Optional<String> type();
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this one anymore, right?

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can we mark it deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should:

  1. Annotate the method with @Deprecated
  2. Add a note to the changelog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@dimas-b dimas-b Mar 23, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Re: warning - IMHO it's not worth the effort (same rationale applies), but if it's simple to implement, why not?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like Option 2 got implemented already 👍

The warning is optional from my POV.

Copy link
Contributor

Choose a reason for hiding this comment

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

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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: Optional<Set<String>> types(); is probably better.

@nandorKollar nandorKollar force-pushed the multiple_event_listener_service_bus branch from 48b5fbd to c9f843b Compare March 16, 2026 22:43

polaris.file-io.type=default

polaris.event-listener.type=no-op
Copy link
Contributor Author

@nandorKollar nandorKollar Mar 16, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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/

@nandorKollar nandorKollar force-pushed the multiple_event_listener_service_bus branch from ee21f22 to 716f6db Compare March 16, 2026 23:13
@nandorKollar
Copy link
Contributor Author

@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.

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.

@nandorKollar nandorKollar marked this pull request as ready for review March 17, 2026 07:22
@adutra
Copy link
Contributor

adutra commented Mar 17, 2026

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.

@nandorKollar
Copy link
Contributor Author

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.

@adutra
Copy link
Contributor

adutra commented Mar 18, 2026

@adutra How about introducing the event listener configuration in a follow-up PR, together with the optimization you mentioned earlier (i.e., avoiding materializing the PolarisEvent)?

In that PR, we could address the following points:

  • Make each event listener configurable. For example, CloudWatch could be configured to receive only a subset of events, while at the same time every event is also persisted in the database.
  • Ensure the event dispatcher only emits events when there is a registered listener for the given event type. This was partially implemented in Create PolarisEvent only when event type is actually supported by listener #3442 — we could revive that work.
  • Avoid assumptions in the event listener implementation about handling only a specific event type.

What do you think?

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?

@nandorKollar
Copy link
Contributor Author

@adutra How about introducing the event listener configuration in a follow-up PR, together with the optimization you mentioned earlier (i.e., avoiding materializing the PolarisEvent)?
In that PR, we could address the following points:

  • Make each event listener configurable. For example, CloudWatch could be configured to receive only a subset of events, while at the same time every event is also persisted in the database.
  • Ensure the event dispatcher only emits events when there is a registered listener for the given event type. This was partially implemented in Create PolarisEvent only when event type is actually supported by listener #3442 — we could revive that work.
  • Avoid assumptions in the event listener implementation about handling only a specific event type.

What do you think?

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 polaris.event-listener.type and adding polaris.event-listener.types. Of course if you think we should avoid event materialization in this PR, I can continue working on that in this change too.

adutra
adutra previously approved these changes Mar 18, 2026
@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 18, 2026
@adutra
Copy link
Contributor

adutra commented Mar 18, 2026

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 polaris.event-listener.type and adding polaris.event-listener.types. Of course if you think we should avoid event materialization in this PR, I can continue working on that in this change too.

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 polaris.event-listener.type.

@nandorKollar
Copy link
Contributor Author

nandorKollar commented Mar 18, 2026

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 polaris.event-listener.type and adding polaris.event-listener.types. Of course if you think we should avoid event materialization in this PR, I can continue working on that in this change too.

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 polaris.event-listener.type.

Should we add it only to the changelog?

EDIT: never mind, found the way to deprecate it in the doc too. :)

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

Sorry about last minute comments. The PR LGTM overall. My only suggestion is to try and add automatic redirects for the old config name.

* identifier.
*/
String type();
Optional<String> type();
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably add a config redirect too.

Cf.

@nandorKollar nandorKollar force-pushed the multiple_event_listener_service_bus branch from 16170de to b570a2e Compare March 20, 2026 13:16
…er_service_bus

# Conflicts:
#	runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java
@flyrain flyrain requested a review from adnanhemani March 21, 2026 06:34

@ApplicationScoped
@DefaultBean
public class PolarisServiceBusEventDispatcher implements PolarisEventDispatcher {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

@flyrain flyrain Mar 21, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

NVM. It seems to be deprecated. I think the word here isn't accurate.

Suggested change
- 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need @ApplicationScoped for this class?

Copy link
Contributor

Choose a reason for hiding this comment

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

IDK off the top of my head 😅 ... but it does not hurt, I'm pretty sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

ServiceProducers have the maybeBootstrap() observer without @ApplicationScoped, so I'd guess it is NOT required here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we drop @ApplicationScoped it might be worth trying to make the observer method static.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a blocker from my POV

Copy link
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

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

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?

@jbonofre jbonofre changed the title Enable multiple event listers to consume for Polaris events Enable multiple event listeners to consume for Polaris events Mar 22, 2026
@jbonofre
Copy link
Member

nit: I updated the PR title (typo on lister instead of listener 😄 ).

@nandorKollar nandorKollar requested review from dimas-b and flyrain March 23, 2026 09:51
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

LGTM, pending resolution of open comment threads 👍 Thanks, @nandorKollar !

@nandorKollar
Copy link
Contributor Author

Great, thanks @dimas-b , @flyrain would you mind also have a look at the latest version? @jbonofre @adutra is there any unresolved comment on your end that I may have forgotten to address?

@adnanhemani
Copy link
Contributor

I will look into this later today, please hold merging until then.

Copy link
Contributor

@adnanhemani adnanhemani left a comment

Choose a reason for hiding this comment

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

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";
Copy link
Contributor

Choose a reason for hiding this comment

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

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(":");
Copy link
Contributor

Choose a reason for hiding this comment

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

spurrious?

package org.apache.polaris.extension.auth.opa.test;

import io.quarkus.test.junit.QuarkusTestProfile;
import io.quarkus.test.junit.QuarkusTestProfile.TestResourceEntry;
Copy link
Contributor

Choose a reason for hiding this comment

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

spurrious?

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.

6 participants