Add Events for PolarisServiceImpl APIs

[adnanhemani]

Adding instrumentation for events for all events defined in PolarisServiceImpl.java. Not all events will be useful and/or necessary - but allow users to plug in with custom event listeners for any of these APIs. The default event listener is a no-op.

The main file to review here is PolarisServiceImpl. All new events are simply just a constructor and getter methods.

[adutra]

A few remarks:

Injecting before/after event logic in every method looks like we are mixing concerns. I think the decorator pattern could make things look a lot nicer.

About the execution flow: how do we handle errors?

E.g. if the listener throws on a before event call, should the execution halt or continue?

Also: if the listener throws on an after event call, should the execution be aborted and the persisted changes rolled back, or should the business changes be kept? IOW, are we OK with weaker delivery guarantees for events (like "at most once")?

[adnanhemani]

Re: execution flow

Personally, I'm okay if we do not give a successful execution back to the user if the event listeners throw an error. For the use case in which customers actually add perhaps additional authorization or validations to a call through an event listener, this is exactly what the customer would expect. This is how event listeners are currently instrumented today as well - and I don't see a solid reasoning to change this paradigm right now. (Event Listeners and their configuration are still the responsibility of the Polaris admin, as this is basically like a custom plugin; additionally, the sample events-to-persistence implementations in #1844 take this into account and purposefully will not throw errors during the execution of the event listener in-line code).

---

Re: decorator

I believe the pattern you are referring to includes defining a @decorator class which would also implement PolarisCatalogsApiService/PolarisPrincipalsApiService/PolarisPrincipalRolesApiService and then use the PolarisServiceImpl class as a delegator? If this is not correct, please do help provide some sample/hints that would make it clearer what you mean.

But going back - in the case of the decorator+delegator, sure this may make this particular file cleaner - but the same amount of code will exist as we will just be moving the event listener hooks to a different file and then calling PolarisServiceImpl from there. If you think that slight amount of cleanliness outweighs the redirection, I'm happy to take this as an action item. Wanted to confirm what you mean before going ahead with an implementation on this.

[adutra]

Couldn't all these event classes be records? We could even group them in one single file instead of having hundreds of similar files around.

[adnanhemani]

Same response as: #1844 (comment)

[snazy]

I think this deserves a dev-ML discussions and consensus on the technical approach.
Alex and Eric already raised concerns about both the general and the specific technical approach

[adnanhemani]

@snazy - to be fair, Eric did not raise any concerns on this PR or approach (have verified offline with him as well). And I don't really see why this warrants a ML-thread. It seems that issues with this PR are implementation-specific, and I don't see a good reason to use ML threads for that (that is what PRs comments should be for).

But I will start a ML thread as per your suggestion - I hope there will be fruitful, outcome-driven conversation that will lead to a quick consensus.

[eric-maynard]

I did have one or two comments on the related PR #1844, but that only to clarify the approach. I am OK with the direction of this PR and the other one; we need events for these operations. If we can reduce the amount of code needed to add the new events or otherwise break down or streamline the process that would be great but I have no blocking concerns at this time.