Event Listeners

[andrew4699]

Implementation of event listeners discussed here.

I decided to keep this implementation generic and not take a dependency on Jakarta Events nor Vertx busses. It's easy to extend this, either within Polaris or in an external PolarisEventListener, and handle events however one wishes.

Some high level notes:

• PolarisEventListener is the main interface with all the event methods such as onBeforeRequestRateLimited
• DefaultPolarisEventListener is an empty implementation which allows users to only partially implement event handlers
• polaris.events.type is the config that lets you specify your event listener implementation

[eric-maynard]

I wonder if we really need the Polaris prefix here.

Also, unfortunately, I think we need javadocs on the methods.

[andrew4699]

Yea we could get rid of the Polaris prefix.

WDYT about @link javadocs that point to the actual event data class? I just didn't wanna duplicate the comment in 2 places. Spark seems to have fairly vague comments on the methods that don't provide much more info than the method name itself, but also Spark doesn't seem to have the detailed comments on the event data classes themselves.

[eric-maynard]

Yeah that's fair, let's just add @links

[adutra]

The "Polaris" prefix would deserve a discuss thread imho. Many components have this prefix, but not all.

[eric-maynard]

Many have it, and many don't -- agreed that we should standardize and agreed that a discussion thread is a good place to do that.

For this individual decision on this PR, I'm not sure a decision either way would require a huge discussion given the current lack of standardization.

[adutra]

The "Polaris" prefix would deserve a discuss thread imho. Many components have this prefix, but not all.

[adutra]

Generally speaking, what are the guarantees about "after" commits? Are they triggered if the operation failed, or just when they succeeded? But then, do we need events for failed operations as well?

I see that AfterAttemptTaskEvent has a boolean success flag, but other "after" events do not. Also, maybe the full throwable is preferable than just a boolean flag.

[eric-maynard]

I agree that we should be as explicit as possible about this, although I think it might be ambitious to guarantee the exact semantics for each event out of the gate. At the very least, we should try our best to define the behavior with tests.

[flyrain]

I think we just need to rename to make it clear. I suggested to use a name like TableCommitted to indicate this event happens only if the table committed successfully. #922 (comment)

[andrew4699]

The Javadoc should describe the semantics to a reasonable extent. It does describe the table commit event as not being emitted during failure, and that each attempt of a task regardless of success is emitted.

Docs aside, there's a question of which semantics we choose to actually implement, and since this is meant to be a broad interface there will be differing opinions on which make the most sense. Rather than trying to pick the perfect one for everyone, folks can always add new events and/or fields to existing events that represent different semantics.

[adutra]

This approach of manually triggering events is fine as a starting point, but I think a more elegant approach would be to create a decorator catalog that would decorate calls to each method with before/after events.

[andrew4699]

Yea that would be interesting. It might not be ideal to couple events to methods though, as that could force you to unnecessarily extract things into a separate method.

[flyrain]

I think adding a new event in Polaris shouldn't break existing listeners. We will need to consider a different design.

[flyrain]

Adding a new event shouldn't require recompilation or redeployment of listeners. How do we archive that with the current design? One of solutions is to remove the specific type in the listener interface, only keep the a generic event type, like the following:

  public void handleEvent(Event event) {
        switch(event.getEventType()) {
            case "BeforeTableCommit":
                handleBeforeTableCommit(event.getPayload());
                break;
           case "TableCommitted":
                handleTableCommitted(event.getPayload());
                break;        
            default:
                // Ignore or log unknown event
        }
    }

[eric-maynard]

I disagree -- if I was maintaining a listener implementation and a new event type was added I would prefer that things fail at build time rather than have my listener silently ignore the new event

[flyrain]

I think it's a common use case that one implementation only cares about a subset of Polaris events, instead of all of them. For example, user A may only care about the table commit event, so it's fine to ignore other events for user A's listener, including new added events.

[eric-maynard]

To Andrew's point, if you want that behavior then you just have your implementation extend DefaultPolarisEventListener.

[flyrain]

My bad. I thought we need to recompile to allow the subclass to get the new methods from the parent class. Java did a good job to keep the binary compatibility, so that the subclass doesn't have to recompile, the caller can still resolve the methods from its parent, https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html. My mindset for this part is still with c++. Sorry for the confusion, @eric-maynard and @andrew4699.

[andrew4699]

I think there's use cases for both wanting to know about new events (ie auditing) and wanting to ignore them (if you only care about a very specific event). Given that, I'm not sure we should sell a single extension point but rather give the option.

I don't think I understand quite enough about how people use dependencies in Java nor the plugin pattern to fully understand the implications of that though. If something is to break, it would ideally happen at compile time and not at runtime.

Also I agree that DefaultPolarisEventListener isn't a great name for an extension point.

[adutra]

The current implementation gives you both options

Does it? If the interface PolarisEventListener is public, it's part of the API. If it's part of the API, you cannot add regular methods to it, even if you implement them in DefaultPolarisEventListener.

[flyrain]

@andrew4699 , two use cases are both valid. However, it shouldn't notify users in a way that breaks existing listeners. If a listener need to include all new events, it has to recompile and redeployment anyways. At the same time, Polaris couldn't just break other listeners who may only care a subset of events.

For example, Polaris will introduce events that covers a wide range of activities like the followings:

1. Table/namespace create/update events
2. Catalog create/update events
3. Principal/role create/update events.

It's common that some listeners only care about table/namespace events, as they are essential for catalog federation, while other listeners only care about principal/role events for identity federation. Adding a new table related event shouldn't break the identity federation listeners.

[andrew4699]

I'm happy to disagree & commit to one option here and do this iteratively, but let me make 1 more case for the current approach:

Spark does almost exactly what this does. SparkListenerInterface is public and they just recommend against using it. The only difference in Spark is that SparkListener (our DefaultPolarisEventListener) is abstract. However, I'm not sure if we have an elegant way to specify "no event listener" with our config singleton pattern, so a non-abstract default listener may actually be needed.

If the interface PolarisEventListener is public, it's part of the API. If it's part of the API, you cannot add regular methods to it, even if you implement them in DefaultPolarisEventListener.

I think as a general rule it's good to apply this principle, but users are getting an explicit warning here. We could make the warning even louder.

[eric-maynard]

SparkListenerInterface is actually private[spark] in Scala, but Scala's private is pretty fake anyway. Java or Scala callers can easily access it.

Does it? If the interface PolarisEventListener is public, it's part of the API. If it's part of the API, you cannot add regular methods to it, even if you implement them in DefaultPolarisEventListener.

The current implementation gives you both the options to:

1. Implement a listener that extends PolarisEventListener and must implement exactly the methods that appear there or fail at build time (or runtime to some ClassNotFoundException if you don't build against the dependency you actually run against).
2. Implement a listener that extends DefaultPolarisEventListener and can optionally override various methods of that class. If new methods appear, your already-built artifact may continue working against a different version of polaris than the one you built against.

In both cases, you also have the option to go into the code, add new events, and handle those events in your listener(s).

It seems like Yufei thinks that people running against a different version than the one they built against will be a common pattern we should support whereas I considered this something to be avoided and discouraged through our design here.

---

In the end I think @flyrain's suggestion that we "hide" the interface like Spark does is fine; expert users who want it can break glass via fork or something and use it anyway. If the DefaultPolarisEventListener being the more accessible extension point turns out to be problematic in the future (e.g. users are surprised by new events being ignored) then we can always expose the interface and update our guidance. In general, I expect anyone deploying a custom listener to be a power-user.

[flyrain]

Curious the use case of this event. I'd assume it's not for the performance instrument.

[flyrain]

Same naming suggestion here, the name would be TableRefreshed

[eric-maynard]

Broadly speaking I think the service doesn't need to be too opinionated on how events might be consumed; the service just emits them. You could imagine something as simple as wanting to keep a counter of how many tables were refreshed in the last 24h.

[andrew4699]

I think adding a new event in Polaris shouldn't break existing listeners. We will need to consider a different design.

That's what DefaultPolarisEventListener is for. Users who are concerned with that issue should extend DefaultPolarisEventListener instead of PolarisEventListener.

[flyrain]

LGTM. Thanks @andrew4699 !

[snazy]

It seems that the event types are missing some important attributes and some event types are exposing implementation specifics.

I'll comment on the dev-ML as well, but overall I don't understand how this is intended to be used ("what are the concrete use cases for this?").

[snazy]

This is test-only code and shouldn't be reachable in production code at all.

[andrew4699]

I agree in principle, but I'm following the same pattern that already exists:

• https://github.com/apache/polaris/blob/main/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/ProductionReadinessChecks.java#L166
• https://github.com/apache/polaris/blob/main/service/common/src/main/java/org/apache/polaris/service/context/TestRealmContextResolver.java

Some beans in test folders don't get resolved.

[snazy]

Sorry, but I don't think that the argument that there's already a bad pattern is good enough to justify this.

[andrew4699]

I'm confused why you approved this earlier then #594

It's not that I agree with the pattern (I don't and have expressed this), it's that maintaining consistency is less confusing to all developers, and this PR doesn't make it materially harder to change the pattern in the future. A separate discussion should happen about changing this but that's out of scope of this PR.

[snazy]

What's the plan/approach for this type when new events are added?
For example: generic tables - are those handled via on*Table* or do those get separate events?

Style: this is rather an interface.

[andrew4699]

I don't think there's anything special about event listeners for the question of "if we implement X, how will that affect Y?" The same principle of maintaining reasonable behavior for existing users should apply.

See #922 (comment) for why it's an ABC instead of an interface.

[snazy]

taskEntityId is a persistence internal property, attempt is actually wrong - see how loadTasks is implemented.
I'm also not sure this event makes sense, given that task handling is incomplete.

[snazy]

Can you elaborate on the use case of this one?

[andrew4699]

Can you elaborate on the attempt being wrong? I don't see what you're referring to.

Here are some threads talking about use cases:

• Event Listeners #922 (comment)
• Event Listeners #922 (comment)

Also the design doc, mailing list thread, and this PR talk about events specifically being intended to be super flexible because different Polaris users will have different needs. Not all events will be useful for everyone.

[snazy]

"Flexible" is not the same as "implementation specific".
Also, there's nothing in the code that could emit this one.
So this one should actually be removed.

If there's a specific use case, let's discuss the use case specifically and please not add stuff that's not usable.

[andrew4699]

Also, there's nothing in the code that could emit this one.

https://github.com/apache/polaris/blob/ab2560d27fb9df225f1a7ee1cdca27b20bfc8676/service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java#L165

specific use case

Logging.

[snazy]

Same as below - logging isn't a good use case.
Logging services already allow that - just implement the necessary logging in the code.

[andrew4699]

#922 (comment)

[snazy]

What's the use case of this one and how would an implementation behave in case of a DoS attack attempt?

[andrew4699]

See #922 (comment)

Can you elaborate on your DoS concerns? If you're using the no-op listener, nothing changes. It's up to you to respond to events in your preferred way.

[snazy]

I'd like to counter the question: What's the concrete use case for this one?

[andrew4699]

Logging.

[snazy]

I don't think that "logging" is a good enough reason for an event.
Logging can happen from the limiter, right?

[andrew4699]

People have different logging requirements. Logs are a controversial change, which you explained elegantly here.

[snazy]

Sorry, I'm strongly -1 on this event. DoS w/ millions or billions of requests are already an issue - this event just makes such a situation worse.

[andrew4699]

Can you elaborate on your DoS concerns? If you're using the no-op listener, nothing changes. It's up to you to respond to events in your preferred way.

[snazy]

That's the only one that's there - "noop". The code yields exactly nothing. The events can't be consumed by anybody. I would really like to see concrete use cases for Apache Polaris users, how the architecture would look like and how things are integrated.

The missing attributes that I noticed let me think that the consuming side (aka use cases) hasn't been thought through.

[andrew4699]

The code yields exactly nothing. The events can't be consumed by anybody.

Isn't exactly the opposite true? After this PR, events can be consumed by everybody!

I've listed demonstrable use cases for each event in the design doc.

[snazy]

Don't failures deserve to be emitted?

[andrew4699]

Maybe. Events are supposed to be flexible and cover a variety of needs, so there isn't a clear minimum nor maximum for what should be implemented. This PR aims to introduce the concept and cover a large surface area, but by design it's easy to add more features.

[snazy]

What's the point of having an "onBefore*" but not always an "onAfter*" then?

[andrew4699]

The point is that there's no common list of events that cover what everyone needs, so lets implement a big chunk of them. The beauty of open source is that if someone wants more events, it's easy for them after this PR to add them :)

[snazy]

Sorry, strong -1 on "lets implement a big chunk just because it's possible".

[snazy]

but it's impossible to know all concrete use cases

Exactly that! But to me the justification "let's implement everything we can think of because it's possible" does not work. Having code just because we can have code - sorry, no.

[andrew4699]

Again, "because it's possible" is not the justification. There are use cases for each event implemented here, and there should be a demonstrable one for events added in the future. The design doc says this:

The intent is for the list of events to grow with Polaris and for adding an event to feel like a relatively lightweight change, compared to refactoring an entire component into one/many swappable interfaces. Not everything should be an event though; there should be at least 3 criteria for adding a new event:

• Difficult to achieve through other means (configuration/dependency injection/etc)
• A use case can be demonstrated, although it may not be obviously useful to all users of the OSS project
• Cannot be folded into existing event listeners

All 3 criteria are true for events in this PR.

[snazy]

Again, "because it's possible" is not the justification.

You mentioned above: there's no common list of events that cover what everyone needs, so lets implement a big chunk of them

[snazy]

I'm asking for concrete requirements.

[andrew4699]

so lets implement a big chunk of them

Replace "them" with "events with demonstrable use cases".

I'm asking for concrete requirements.

Ok, I've added a new column to the design doc with example use cases for each event.

[eric-maynard]

With the updated examples in the doc, this LGTM!

[eric-maynard]

To quickly capture my understanding wrt. how this relates to, and differs from, the events APIs proposed in Iceberg and the observability APIs proposed for Polaris, I mocked up an example of what an architecture might look like:

The tl;dr for readers confused about how these events accomplish observability is that they don't... they allow Polaris to emit events to some external source which might later be used to power observability. They also allow you to do other handy things -- like file a Jira if more than 100 tables are created in a given namespace -- if you're willing to write a custom event listener.

[andrew4699]

Yes exactly. Also solving atomicity (ie of table commit + event) is out of scope of this PR. The model for these event listeners is that they are as if you are adding new specific code to Polaris. Like Polaris, the code running in these event listeners can be interrupted at any time, due to ie a process crash. The external-facing events implementation can choose to build on top of this or not.

[eric-maynard]

@snazy given the examples added to the doc in March are there still specific events you have concerns about emitting?

@snazy can you take a second look to confirm whether you still have concerns about certain events? I looked through the linked doc and the example there make sense to me. In the meantime, we have added a lot of new actions where we should consider putting an event listener hook e.g. policy creation.