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
