flyingImer commented on code in PR #4616:
URL: https://github.com/apache/polaris/pull/4616#discussion_r3365866144
##########
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:
This is the part that makes me think the listener contract is changing.
Since each delivery is scheduled independently, the same listener can now be
invoked concurrently if the pool has multiple threads. Is that intended to be
part of the listener contract?
If yes, I think PolarisEventListener should explicitly say implementations
must be thread-safe and should not rely on callback ordering. If no, maybe we
need per-listener serialization on top of this executor.
--
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]
