I think we may be making too many assumptions about the semantics of an event being emitted and letting that paralyze us. If you're looking for an event to be emitted *iff* a table change is committed to the metastore, the current system simply does not support that use case:
- BeforeTableCommittedEvent tells you how many times a client made an API call that triggers a table commit and the call passed auth, etc. - AfterTableCommittedEvent tells you how many times the commit succeeded and the server attempted to return a response to the client. Wanting to know information about these events (how many times a client attempted to commit to a table, or how many times the server attempted to return a response about a table commit to a client) is a valid use case. We use these events today to get metrics about the kinds of commits being made. The fact that these events don't tell you with perfect accuracy when changes are made to the metastore does not by itself mean there is never any value in persisting them. In fact, they were never meant to do so in the first place. We could certainly add a third event in the future -- OnTableCommittedEvent -- which has the kind of transactional semantics you're describing. Some EventListeners, those that use the same DB as the metastore, could transactionally handle this event and make changes to the metastore like you describe. This would require significant code changes. Other EventListeners, such as one that sends events into a message queue, inherently won't be able to support that transactionality even if this third event is added. I don't see this as a reason to block persisting the existing events. --EM On Tue, Jun 24, 2025 at 1:27 AM Alex Dutra <alex.du...@dremio.com.invalid> wrote: > Hi, > > To be honest, the decorator approach is nice to have, but I could live > without it. > > What worries me the most right now is the design decision around > "after" events. That a "before" event could exist without the > corresponding "after" one, or even without the corresponding update to > the metastore, is of concern. > > Thanks, > Alex > > > On Mon, Jun 23, 2025 at 10:35 PM Adnan Hemani > <adnan.hem...@snowflake.com.invalid> wrote: > > > > > > > > I'm not sure the pros and cons about these two concerns were discussed. > > > Those are general decisions that IMO require a bigger audience than a > > > single code-level comment in one PR. > > > > > > I understand where you're coming from. To keep things moving, I'm > proposing > > a one-week time limit on this discussion—unless there’s still active > > engagement at that point. The goal is to avoid letting the conversation > > drag on and continue blocking the PR if no new perspectives are being > > shared beyond those who have already contributed. I’ll consider this time > > limit in effect unless there’s explicit and valid pushback. > > > > I do also prefer delegation over adding "all concerns to a single > > > class". It simplifies testing and separates concerns, making > > > failure-handling much easier, prevents leaking event-specifics into the > > > actual implementation and also prevents new service methods/functions > > > from not being decorated. > > > > > > Got it, I think there are now two votes for this (you and Alex). In this > > case, I can start work on this and modify the PR in the upcoming days. > > > > Not having any error handling leads to unnecessary ambiguity. Nobody can > > > certainly reason about the cause why some "after event" is not there. > > > There are legit reasons for and against having that. But again, it's a > > > general and "far reaching" decision. > > > > > > I've considered this perspective as well, but I believe that Events are > > intentionally designed without error handling. If someone uses Events > for a > > valid case where throwing errors is appropriate - like adding extra > > validation to an API call - introducing error handling could break that > > pattern. So the real question is: is there a compelling reason to change > > the current behavior and add error handling to Events? The responsibility > > to prove that this change is clearly beneficial lies with those proposing > > it. > > > > Best, > > Adnan Hemani > > > > On Mon, Jun 23, 2025 at 3:33 AM Robert Stupp <sn...@snazy.de> wrote: > > > > > I'm not sure the pros and cons about these two concerns were discussed. > > > Those are general decisions that IMO require a bigger audience than a > > > single code-level comment in one PR. > > > > > > I do also prefer delegation over adding "all concerns to a single > > > class". It simplifies testing and separates concerns, making > > > failure-handling much easier, prevents leaking event-specifics into the > > > actual implementation and also prevents new service methods/functions > > > from not being decorated. > > > > > > Not having any error handling leads to unnecessary ambiguity. Nobody > can > > > certainly reason about the cause why some "after event" is not there. > > > There are legit reasons for and against having that. But again, it's a > > > general and "far reaching" decision. > > > > > > > > > On 21.06.25 00:13, Adnan Hemani wrote: > > > > Hi Robert, > > > > > > > > I’ve already responded to those concerns < > > > https://github.com/apache/polaris/pull/1904#discussion_r2153824586> on > > > the PR itself, prior to your comment asking for this email thread. > Please > > > take a look and feel free to respond either on the PR or this email > thread, > > > now that we’ve already gotten it going. > > > > > > > > To be clear, I’m not opposed to taking feedback on either of these > > > topics - but reiterating already responded-to threads with no new > > > information/opinions/suggestions does not help drive the discussion > forward > > > IMO. > > > > > > > > Best, > > > > Adnan Hemani > > > > > > > > > > > >> On Jun 20, 2025, at 7:29 AM, Robert Stupp <sn...@snazy.de> wrote: > > > >> > > > >> Thanks for bringing this up. > > > >> > > > >> I referenced the concerns that were mentioned on the PR about > > > >> * the approach not using a `@Decorator`, mixing concerns. > > > >> * exception/failure handling. > > > >> > > > >> For me, these are important topics that need to be considered in the > > > PR. It would be good to respect those concerns before we start > reviewing > > > this big-ish change, as there are possibly options to shrink it > (immutables > > > as an option?) and also consider exception/failure handling. > > > >> > > > >> > > > >> On 19.06.25 04:48, Adnan Hemani wrote: > > > >>> Hi all, > > > >>> > > > >>> As per the comment on GitHub PR #1904 < > > > > https://www.google.com/url?q=https://github.com/apache/polaris/pull/1904&source=gmail-imap&ust=1751034561000000&usg=AOvVaw0kTwOl-IaXvgc8vVItGrHI > >, > > > I am opening this mailing thread to discuss the PR implementation. > This PR > > > is only adding instrumentation for additional events in > > > `PolarisServiceImpl.java`. There is no additional business logic as > part of > > > this PR. > > > >>> > > > >>> There was one noted (and responded) remark about execution flow and > > > one remark regarding cosmetic changes to the code, which is awaiting > follow > > > up by adutra@. > > > >>> > > > >>> I will let snazy@ take the floor from here as he requested this > mail > > > thread without being specific on his concerns on the PR comment. > > > >>> > > > >>> Best, > > > >>> Adnan Hemani > > > > > > > >
