Skip to content

[GOBBLIN-2255] Temporal workflow fast failure#4174

Merged
abhishekmjain merged 4 commits intoapache:masterfrom
adithya2754:adithya2754/temporal-fast-fail
Mar 11, 2026
Merged

[GOBBLIN-2255] Temporal workflow fast failure#4174
abhishekmjain merged 4 commits intoapache:masterfrom
adithya2754:adithya2754/temporal-fast-fail

Conversation

@adithya2754
Copy link
Contributor

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

  • My PR addresses the following GOBBLIN-2255 issues and references them in the PR title.

Description

  • Here are some details about my PR, including screenshots (if applicable):
    Temporal workflow goes on to re-attempt even when there is a non intermittent issue - like when user Quota is Exceeded, where successive attempts will not stop it from failure.

This is to introduce fast failure when we see Quota Exceeded Exceptions during the workflow

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    Tested for a flow

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Comment on lines +97 to +99
} catch (IOException e) {
Throwable cause = e.getCause();
if (cause instanceof DSQuotaExceededException || cause instanceof NSQuotaExceededException) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we create a set of non retryable exceptions and check on that instead of writing each exception here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that would be better ✅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although trivial added unit tests for util class :)

return Optional.empty();
}
for (Class<? extends Exception> exClass : EXCEPTIONS) {
if (exClass.isInstance(throwable)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also check for cause recursively, throwable has getClause() which returns null when there is no more underlying cause.

public synchronized Throwable getCause() {
    return (cause==this ? null : cause);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will help in identifying cases where the nonRetryableException is nested

@abhishekmjain abhishekmjain merged commit 90399ee into apache:master Mar 11, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants