adutra commented on code in PR #4616:
URL: https://github.com/apache/polaris/pull/4616#discussion_r3373223943
##########
runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListeners.java:
##########
@@ -81,7 +86,23 @@ private void deliverEvent(
PolarisEvent event, String listenerName, PolarisEventListener listener) {
LOGGER.debug("Delivering {} event to listener '{}' ({})", event.type(),
listenerName, listener);
try {
- listener.onEvent(event);
+ executor.execute(
+ () -> {
+ LOGGER.debug(
+ "Delivering {} event to listener '{}' ({})", event.type(),
listenerName, listener);
+ try {
+ listener.onEvent(event);
+ } catch (Exception e) {
+ LOGGER.error(
+ "Error while delivering {} event to listener '{}' ({})",
+ event.type(),
+ listenerName,
+ listener,
+ e);
+ }
+ LOGGER.debug(
+ "Delivered {} event to listener '{}' ({})", event.type(),
listenerName, listener);
+ });
Review Comment:
Thanks @flyingImer that's indeed a valid concern. The Vert.x event bus
guarantees ordered message delivery to its consumers. Offloading the delivery
to a separate executor indeed breaks this (implicit) contract.
Assuming that we want to preserve strictly serialized per-listener event
delivery: I see two ways to fix this.
1. Use Vert.x `WorkerExecutor` which offers an `executeBlocking` method that
can optionally guarantee ordered delivery. But the drawback of this approach is
that we'd need one `WorkerExecutor` per listener.
2. For each listener, wrap the existing `ManagedExecutor` with a simple
`Executor` that serializes execution of tasks leveraging its own internal queue.
I am going to explore the second option since I think it's preferable to
have a global executor, rather than N executors, one per listener.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
