ferenc-csaky commented on PR #20265: URL: https://github.com/apache/flink/pull/20265#issuecomment-1233134897
> > One general question: is catching Throwable a good thing here? I see that there are rethrowIfFatalError calls and it is a common pattern in the existing code, but that method only cares about a subset of errors (e.g. it ignores OOM or StackOverlflow). > > Not sure what you in-depth mean but the original and my current intention here is clear. Catch and log everything because it's always good if it blows up with log entry. On the other hand non-fatal exceptions can be intermittent, retry comes at later timepoint where it could be successful. Fatal exceptions however must stop the workload. > > If you think there is a bug in `rethrowIfFatalError` then a separate jira would be better to discuss. The reasons are simple from my perspective: > > * Several execution paths are involved since it's widely used > > * Rewriting rethrowIfFatalError` logic because it's maybe faulty would be overkill Me neither, I do not have in-depth knowledge, I just wanted to mention this, because IMO catching `Throwable` in general is not a good practice, cause `Error`s are non-recoverable events and the stack trace will be printed at anyways. I'm sure this is not really part of this PR's context and following the already existing practice is the way to go, I just started to wonder is it right in the first place or not. :) The logic itself LGTM for me! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
