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