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-2190] Implement ActivityType & add HeartBeat for Temporal Activities #4093

Merged
merged 12 commits into from
Feb 25, 2025

Conversation

Blazer-007
Copy link
Member

@Blazer-007 Blazer-007 commented Jan 23, 2025

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):
    Created ActivityType Enum
    Modified code to create Activity using config based StartToCloseTimeout
    Added Activity Heartbeat which will help if Temporal Worker Running Activity dies
    Refactored the interfaces to pass properties

Tests

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

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"

@Blazer-007 Blazer-007 changed the title [GOBBLIN-XXX] Implement ActivityTimeoutStrategy for Temporal Activities [GOBBLIN-2190] Implement ActivityTimeoutStrategy for Temporal Activities Jan 23, 2025
@Blazer-007 Blazer-007 marked this pull request as ready for review January 23, 2025 12:34
@Blazer-007 Blazer-007 changed the title [GOBBLIN-2190] Implement ActivityTimeoutStrategy for Temporal Activities [GOBBLIN-2190] Implement ActivityConfigurationStrategy for Temporal Activities Jan 31, 2025
@vsinghal85
Copy link
Contributor

As discussed offline, please add the testing details

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 29.00000% with 71 lines in your changes missing coverage. Please review.

Project coverage is 43.09%. Comparing base (adef734) to head (38cde71).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
...ting/workflow/AbstractNestingExecWorkflowImpl.java 0.00% 12 Missing ⚠️
...al/util/nesting/work/NestingExecWorkloadInput.java 0.00% 10 Missing ⚠️
.../ddm/workflow/impl/ExecuteGobblinWorkflowImpl.java 0.00% 9 Missing ⚠️
...dm/workflow/impl/ProcessWorkUnitsWorkflowImpl.java 0.00% 8 Missing ⚠️
...temporal/ddm/activity/impl/CommitActivityImpl.java 0.00% 7 Missing ⚠️
...poral/ddm/activity/impl/GenerateWorkUnitsImpl.java 0.00% 7 Missing ⚠️
...emporal/ddm/activity/impl/ProcessWorkUnitImpl.java 0.00% 7 Missing ⚠️
...impl/NestingExecOfProcessWorkUnitWorkflowImpl.java 0.00% 3 Missing ⚠️
.../loadgen/launcher/GenArbitraryLoadJobLauncher.java 0.00% 3 Missing ⚠️
...ingExecOfIllustrationItemActivityWorkflowImpl.java 0.00% 2 Missing ⚠️
... and 3 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4093      +/-   ##
============================================
- Coverage     45.38%   43.09%   -2.29%     
+ Complexity     3192     2467     -725     
============================================
  Files           696      511     -185     
  Lines         26628    21499    -5129     
  Branches       2655     2457     -198     
============================================
- Hits          12085     9265    -2820     
+ Misses        13542    11289    -2253     
+ Partials       1001      945      -56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Blazer-007 Blazer-007 requested a review from Copilot February 6, 2025 07:11
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 19 changed files in this pull request and generated 1 comment.

Files not reviewed (14)
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/workflow/impl/ProcessWorkUnitsWorkflowImpl.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/workflow/impl/ExecuteGobblinWorkflowImpl.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/launcher/ProcessWorkUnitsJobLauncher.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/workflow/impl/NestingExecOfProcessWorkUnitWorkflowImpl.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/work/WUProcessingSpec.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/workflow/impl/CommitStepWorkflowImpl.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/workflow/CommitStepWorkflow.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/util/nesting/workflow/NestingExecWorkflow.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/util/nesting/workflow/AbstractNestingExecWorkflowImpl.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/workflow/ProcessWorkUnitsWorkflow.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/loadgen/workflow/impl/NestingExecOfIllustrationItemActivityWorkflowImpl.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/loadgen/launcher/GenArbitraryLoadJobLauncher.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/GobblinTemporalConfigurationKeys.java: Evaluated as low risk
  • gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/activity/ActivityConfigurationStrategy.java: Evaluated as low risk
Comments suppressed due to low confidence (3)

gobblin-temporal/src/test/java/org/apache/gobblin/temporal/ddm/util/TemporalActivityUtilsTest.java:39

  • Mocking ActivityType is unnecessary and might lead to misleading test results. Use a real ActivityType value instead.
ActivityType activityType = Mockito.mock(ActivityType.class);

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/util/TemporalActivityUtils.java:49

  • Add validation to ensure that every ActivityType has a corresponding strategy to prevent runtime errors.
private static final Map<ActivityType, ActivityConfigurationStrategy> activityConfigurationStrategies = new HashMap<>();

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/util/TemporalActivityUtils.java:29

  • Correct the spelling of 'Temporal' in the comment on line 30.
import lombok.experimental.UtilityClass;

@Blazer-007 Blazer-007 changed the title [GOBBLIN-2190] Implement ActivityConfigurationStrategy for Temporal Activities [GOBBLIN-2190] Implement ActivityType & add HeartBeat for Temporal Activities Feb 19, 2025
@khandelwal-prateek
Copy link
Contributor

Please update the description explaining the change and what it helps with

@Blazer-007 Blazer-007 merged commit f2bcdc7 into apache:master Feb 25, 2025
6 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.

4 participants