Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GOBBLIN-2153]added search attributes to Temporal workflows #4052

Merged
merged 11 commits into from
Sep 17, 2024

Conversation

pratapaditya04
Copy link
Contributor

@pratapaditya04 pratapaditya04 commented Sep 11, 2024

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

Description

  • Here are some details about my PR, including screenshots (if applicable):
    Added flowGroup.flowName and user.to.proxy as SearchAttributes to filter Temporal Flows in the UI

Tests

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

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 46 to 48
Map<String, Object> convertedAttributes = new HashMap<>();

convertedAttributes.putAll(searchAttributes);

Choose a reason for hiding this comment

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

It seems like the intention of this method is to convert the input map Map<String, List<?>> into a Map<String, Object> without transforming the list values into single objects. This method seems to be written for setSearchAttributes on ChildWorkflowOptions in AbstractNestingExecWorkflowImpl(which expects Map<String, Object>), we can simplify the implementation by directly using new HashMap<>(searchAttributes). This would achieve the same result more efficiently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

simplified the conversion, however have kept this function as we need validations as new HashMap<>(searchAttributes) throws NPE in case passed parameter is null

Comment on lines 79 to 81
} catch (Exception ignored) {
log.error("Exception occured during performing Work", ignored);
return CommitStats.createEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale for this try catch? Seems dangerous to ignore exceptions during processworkunits step, shouldn't we want it to fail loudly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Help.loadJobState(workSpec, Help.loadFileSystem(workSpec)) throws a checked exception which needs to be catched, however have reduced the scope of the try catch block to this line

Copy link
Contributor

Choose a reason for hiding this comment

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

If it throws an exception when loading the jobState (which I presume would lead to an empty jobState) we should just throw an ApplicationException in this scenario, so that we can push the Temporal framework to retry the workflow rather than progressing silently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, addressed

@Will-Lo Will-Lo merged commit 8a8d230 into apache:master Sep 17, 2024
6 checks passed
Copy link
Contributor

@Will-Lo Will-Lo left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

4 participants