Hi all, Thanks for your responses, JB and Russell. I agree with the points made by Russell: * There are no major API changes/additions introduced that would require a new interface-only PR. * Taking Russell’s suggestions do not change anything that was mentioned in the first email in this thread.
As part of the delegator pattern refactor I have committed to do on PR#1904, I will firstly introduce a (new) PR to make (no-op) delegator classes for all the major Service APIs that are in Polaris based on this feedback from Russell. Then, I can re-introduce PR#1904 with the new Events Delegator. Best, Adnan Hemani > On Aug 21, 2025, at 8:47 AM, Russell Spitzer <russell.spit...@gmail.com> > wrote: > > I think the problem here is that we already have merged some apis, see > PolarisEventListener > <https://www.google.com/url?q=https://github.com/apache/polaris/blob/main/runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListener.java&source=gmail-imap&ust=1756396128000000&usg=AOvVaw2hdz6gzSGLsSorYUn3gOdy>. > 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://www.google.com/url?q=https://github.com/apache/polaris/pull/1904&source=gmail-imap&ust=1756396128000000&usg=AOvVaw2k-D9sN19JSXK7nkfvfDQT>: >> 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://www.google.com/url?q=https://github.com/apache/polaris/pull/1844&source=gmail-imap&ust=1756396128000000&usg=AOvVaw0xoAGoP_bKPkfBdKjWCqgh> >> 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://www.google.com/url?q=https://github.com/apache/polaris/pull/1965&source=gmail-imap&ust=1756396128000000&usg=AOvVaw1t2kJ7pb74RoBigmBamPhe> >> 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) {} >>> … >>> ``` >>
