I think the problem here is that we already have merged some apis, see PolarisEventListener <https://github.com/apache/polaris/blob/main/runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListener.java>. The Pr's above extend that pattern and add more. If I understand Robert's request, he would like to remove the previously merged APIs and would like the whole interface to go back to the drawing board to switch to a Stateful Event handling system. So I'm not sure an "API Only" PR would help in this case since the objection is not just to these changes, but to previously merged ones.
Currently we have a callback style pattern, and I wouldn't consider adding more callbacks as normally requiring any sort of discussion (each api is just "onBefore", "onAfter" although I guess we could argue about their object contents but I don't think they are that broad). Adding additional callbacks for errors/exceptions also wouldn't be a huge deal and could always be done later as well. The change to stateful event APIs I believe is to allow for a plugin which allows injection of external state into the event processing as it takes place. I'm assuming that if you perhaps had an implementation specific identifier you wanted for an event which was required for performance, you would need this sort of API. If you just needed to track an event, RequestID is available but I'm guessing there is a system here which wouldn't be able to easily correlate items based on a system provided identifier? If there are other use cases I'd like to know about them since I'm also wary of putting too much event processing logic into Polaris itself so I'm skeptical about the cost/benefit of allowing this. I am confident that even if we did decide as a community to go ahead with a Stateful event listener API, we wouldn't have to drop the existing apis since it would be trivial to automatically wrap a callback event listener with a stateful one. So I probably would also not block the existing PR's unless we were, as I noted above, going to rip out all the event apis. I think if we did want to switch to a Stateful event handling that it's probably on those requesting a different API layout to propose that and write up the design document on how that pattern would be helpful to users of Polaris and what the pros and cons would be especially in context of what we already have in the project. I do think one pretty good path forward though would be to first develop the interfaces and patterns required to fully delegate to PolarisCoreImpl as Robert requested. If we do that, then any implementer could just load their own wrapping class which could use whatever eventing pattern they like even if it doesn't match Polaris official apis (StatefulEventDelegatingPolarisCore or CallbackEventDelegatingPolarisCore - These could always be built on the vendor side as well). There are of course some cons to this approach as well by adding yet another level of inter-direction but if we add that layer, and only some users want call backs and not allow stateful event handling, they could do that without a lot of fuss just by making their own delegating class. On Thu, Aug 21, 2025 at 1:04 AM Jean-Baptiste Onofré <j...@nanthrax.net> wrote: > Hi Adnan > > Thanks for the message with the list of PRs and details. > > May I propose to create another PR focusing on API updates ? I quickly > took a look on the PRs and my understanding is that these PRs update > the API. > I propose we start with a simple PR focusing on API change (easily to > review and discuss). The purpose is to give "visibility" to the > community and speed up on the implementation side as soon as we have > consensus on API. > > Thoughts ? > > Thanks ! > Regards > JB > > On Tue, Aug 19, 2025 at 9:08 AM Adnan Hemani > <adnan.hem...@snowflake.com.invalid> wrote: > > > > Hi all, > > > > Thanks to those who could make the Polaris Community Sync regarding the > Events API on 2025-08-18. Below are the notes (and my further thoughts > regarding the discussion topics) from the meeting. As discussed (without > any expressed disagreement), there will be a one-week window (ending on > 2025-08-25) to express any blocking concerns with any of the approaches or > proposals, and a further one week window to then suggest any approaches to > resolve these concerns (ending on 2025-09-01). Not responding within these > time windows will be considered “reached consensus.” Please ensure any > responses clearly indicate which proposal/PR the comments or feedback are > for - or feel free to comment/review on the PRs directly! > > > > Thanks to the whole community for helping to move the Events feature > forward! > > > > Proposal #1 - PR #1904 <https://github.com/apache/polaris/pull/1904>: > Additional Instrumentation for New Events > > > > As discussed at the sync, there are no notable changes to the Events and > Event Listener interfaces within this PR - this proposal solely adds new > instrumentation for events using the existing interfaces and emulates > current implementations of said interfaces. > > > > Regarding concerns of adding too many files within this PR: I have > committed to refactoring all Event records into a shared Java class. I will > push this change with the next revision. > > Regarding concerns of adding too much code within the business logic: I > can commit to refactoring `PolarisServiceImpl.java` to create a delegator > pattern where each method would emit events in a separate Java class and > delegate back to `PolarisServiceImpl.java`. This could look something like > Appendix 1. This pattern should satisfy the ability for someone (in a > future PR) to implement `onError…` methods, in case we would like to do > error handling using the Events API. To be clear, error handling was not a > stated use-case of the Events API - but looks to be a straightforward > extension. > > Regarding concerns of the “lack of functionality” to correlate > before/after events together: > > As stated at the sync, this is a false statement. There is a way to do > this already - but it is up to the Event Listener implementation to make > this happen. > > All to-date-publicized alternatives would also need to handle the logic > of correlating before/after events within the Event Listener > implementation, and so I cannot consider them as “strictly better”. The > alternatives would also change the easy-to-implement interfaces to more > complex ones, which is a significant drawback to this feature. Any > interface that requires user implementation should always be as simple as > possible. > > There is not any concrete/known use case for this requirement, nor was > it considered an original goal of the Events API. Given all the above, it > is currently hard for me to understand this as a blocker for merging > further event instrumentation with the current set of interfaces. > > On a related note, any major changes to the Event interfaces would > already need to be considered as “breaking changes,” as it is already > released and public within our Polaris 1.0.0 release. > > There is also a potential solution to pass in the “before” event ID with > a small backwards-compatible change to the Events Listener interface > (Appendix 2). I’m happy to field questions on this proposed change in an > unrelated thread to PR #1904, either on Slack or in a different mail thread. > > > > Many thanks to Russell Spitzer for helping to identify some of the > solutions listed above! > > > > — > > > > Proposal #2 - PR #1844 <https://github.com/apache/polaris/pull/1844> > Add Polaris Events to Persistence > > > > This was not thoroughly discussed during the Community Sync due to lack > of time, but was touched upon briefly whenever it was relevant as a new > sample implementation of the Events Listener interface. > > > > There are currently no unaddressed concerns or comments on this PR. I am > surfacing this as a way for a “last call” for feedback before this gets > merged. > > > > — > > > > Proposal #3 - PR #1965 <https://github.com/apache/polaris/pull/1965> > Add Event Listener Implementation using AWS CloudWatch as a event sink > > > > This was not mentioned during the community sync due to lack of time as > well. The goal is hopefully to repeat the patterns in this PR for other > cloud service provider logging products. > > > > There are a few minor comments remaining on this PR, which I will > address in the upcoming days. There are plans to merge this PR following > that resolution. This is an also being surfaced for a “last call” for > feedback. > > > > Regards, > > Adnan Hemani > > > > — > > > > Appendix 1: > > > > EventingPolarisServiceImpl.java: > > ``` > > public class EventingPolarisServiceImpl implements > PolarisCatalogsApiSerivce, PolarisPrincipalsApiService, > PolarisPrincipalsRolesApiService { > > private final PolarisServiceImpl delegate; > > private final PolarisEventListener listener; > > > > public … createCatalog(…) { > > listener.onBeforeCreateCatalog(…); > > try { > > delegate.createCatalog(…); > > listener.onAfterCreateCatalog(…); > > } catch (Exception e) { > > listener.onExceptionCreateCatalog(…); > > } > > } > > } > > ``` > > > > Appendix 2: > > > > EventingPolarisServiceImpl.java: > > ``` > > public class EventingPolarisServiceImpl implements > PolarisCatalogsApiSerivce, PolarisPrincipalsApiService, > PolarisPrincipalsRolesApiService { > > private final PolarisServiceImpl delegate; > > private final PolarisEventListener listener; > > > > public … createCatalog(…) { > > String eventId = listener.onBeforeCreateCatalog(…); > > try { > > delegate.createCatalog(…); > > listener.onAfterCreateCatalog(<event>, eventId); > > } catch (Exception e) { > > listener.onExceptionCreateCatalog(<event>, > eventId); > > } > > } > > ``` > > > > PolarisEventListener.java: > > ``` > > … > > // The Event Listener implementation would then be in charge of > generating the eventId and returning it… > > public String onBeforeCreateCatalog(…) {} > > > > // …as well as using it further (if it would like). > > public void onAfterCreateCatalog(<event>, String eventId) {} > > > > // We can achieve backwards compatibility through sending in `null` as > the event ID. > > // Event Listener implementations would need to handle this case. > > // Additionally, we can mark this as Deprecated, if it makes for easier > maintenance > > public void onAfterCreateCatalog(<event>) { > > onAfterCreateCatalog(<events>, null); > > } > > > > // No need to handle backwards-compatibility here as this is not yet > created and is still a proposal. > > public void onExceptionCreateCatalog(<event>, String eventId) {} > > … > > ``` >
