scwhittle commented on code in PR #40955:
URL: https://github.com/apache/beam/pull/40955#discussion_r4452753471
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/WorkCancellingException.java:
##########
@@ -36,8 +39,9 @@ public WorkCancellingException(Throwable cause) {
/** Returns whether an exception was caused by a {@link
WorkCancellingException}. */
public static boolean isWorkCancellingException(Throwable t) {
+ Set<Throwable> seen = Collections.newSetFromMap(new IdentityHashMap<>());
@Nullable Throwable throwable = t;
- while (throwable != null) {
+ while (throwable != null && seen.add(throwable)) {
Review Comment:
do we actually have such cycles or is this more theoretical?
Is it worth avoiding the allocation in the case the input throwable doesn't
have a cause?
I think there are other such cases where we iterate a throwable chain.
Maybe we should have a helper taking the throwable class and doing this
iteration, and use it in other cases too.
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/work/processing/failures/WorkFailureProcessor.java:
##########
@@ -173,6 +175,17 @@ private RetryEvaluation evaluateRetry(
return RetryEvaluation.RETRY_LOCALLY;
}
+ if (WorkCancellingException.isWorkCancellingException(parsedException)
+ ||
WorkItemCancelledException.isWorkItemCancelledException(parsedException)) {
Review Comment:
Should we have a helper that checks both? We won't have to iterate the
throwable chain twice
Or should we change WorkCancellingException to WorkItemCancelledException
after setting failed in the Work.java?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
