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 > > > > >
