Hi A PR with delegator classes makes sense to me, and it's a good PR to start with.
Thanks ! Regards JB On Fri, Aug 22, 2025 at 4:39 AM Adnan Hemani <adnan.hem...@snowflake.com.invalid> wrote: > > 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) {} > >>> … > >>> ``` > >> >
