Skip to content

Conversation

@olsoloviov
Copy link
Contributor

This PR implements @adutra's idea from his comment in #3293: #3293 (comment).
I used a bit different approach with Supplier instead of conditional calls to OnEvent(). Polling the listener before emitting the event does not align well with future plans to support multiple active event listeners.

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)

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.

Thanks @olsoloviov for this PR!

The solution looks nice and simple. I have some concerns though.

First off, we are still creating a closure for every event. Since it must capture variables from outside the closure (e.g. the requests and responses), this creates an anonymous class that gets instantiated every time. The cost is low, but is not zero.

But my main concern is that your supplier is not memoized. For now we only have one listener active at a time, but as soon as we get more than one, your proposed design would result in each listener getting a different PolarisEvent instance, which wouldn't be correct and would also be quite expensive.

Polling the listener before emitting the event does not align well with future plans to support multiple active event listeners

In fact, I think it could align, but I agree that it requires some work. One way to solve this problem is as follows:

Declare a new supportedEventTypes() method:

public interface PolarisEventListener {
    default EnumSet<PolarisEventType> supportedEventTypes() {
      return EnumSet.noneOf(PolarisEventType.class); 
    }
    default void onEvent(PolarisEvent event) {}
}

Then:

// For single listener
if (listener.supportedEventTypes().contains(type)) {
    listener.onEvent(new PolarisEvent(...));
}

// For multiple listeners - create event once if any listener supports it
Set<PolarisEventListener> interestedListeners = listeners.stream()
    .filter(l -> l.supportedEventTypes().contains(type))
    .collect(toSet());
if (!interestedListeners.isEmpty()) {
    PolarisEvent event = new PolarisEvent(...);
    interestedListeners.forEach(l -> l.onEvent(event));
}

The above still creates a Set<PolarisEventListener> every time. We can go further and create an intermediate component that manages the event dispatching logic to all subscribers. Then this component could memoize which event types have interested listeners and which don't. This would result in a near-zero cost in the case the event type is not wanted by any listener:

@ApplicationScoped
public class PolarisEventDispatcher {
    
    private final List<PolarisEventListener> listeners;
    private final EnumSet<PolarisEventType> supportedTypes;
    
    @Inject
    public PolarisEventDispatcher(@Any Instance<PolarisEventListener> listeners) {
        this.listeners = listeners.stream().toList();
        
        // Memoize at startup: which event types have at least one interested listener
        this.supportedTypes = EnumSet.noneOf(PolarisEventType.class);
        for (PolarisEventListener listener : this.listeners) {
            this.supportedTypes.addAll(listener.supportedEventTypes());
        }
    }
    
    public boolean hasListeners(PolarisEventType type) {
        return supportedTypes.contains(type);
    }
    
    public void fireEvent(PolarisEvent event) {
        for (PolarisEventListener listener : listeners) {
            if (listener.supportedEventTypes().contains(event.type())) {
                listener.onEvent(event);
            }
        }
    }
}

switch (type) {
case AFTER_CREATE_TABLE -> handleAfterCreateTable(eventSupplier.get());
case AFTER_CREATE_CATALOG -> handleAfterCreateCatalog(eventSupplier.get());
default -> {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe keep the comment?

public interface PolarisEventListener {

default void onEvent(PolarisEvent event) {}
default void onEvent(PolarisEventType type, Supplier<PolarisEvent> eventSupplier) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this is not binary-compatible. We would need to keep the old method around. e.g.

default void onEvent(PolarisEventType type, Supplier<PolarisEvent> supplier) { onEvent(supplier.get()); }

@Deprecated
default void onEvent(PolarisEventType type, Supplier<PolarisEvent> eventSupplier) {}

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.

2 participants