[GOBBLIN-2190] Implement ActivityType & add HeartBeat for Temporal Activities #4093

Blazer-007 · 2025-01-23T05:05:36Z

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 JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-2190

Description

Here are some details about my PR, including screenshots (if applicable):
Implemented ActivityConfigurationStrategy
Created ActivityType Enum
Refactored the interfaces to pass properties

Tests

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

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"

...al/src/main/java/org/apache/gobblin/temporal/ddm/activity/ActivityConfigurationStrategy.java

...oral/src/main/java/org/apache/gobblin/temporal/ddm/workflow/impl/CommitStepWorkflowImpl.java

.../org/apache/gobblin/temporal/ddm/workflow/impl/NestingExecOfProcessWorkUnitWorkflowImpl.java

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

...obblin/temporal/loadgen/workflow/impl/NestingExecOfIllustrationItemActivityWorkflowImpl.java

vsinghal85 · 2025-02-03T04:28:58Z

.../org/apache/gobblin/temporal/ddm/workflow/impl/NestingExecOfProcessWorkUnitWorkflowImpl.java


  @Override
-  protected Promise<Integer> launchAsyncActivity(final WorkUnitClaimCheck wu) {
-    return Async.function(activityStub::processWorkUnit, wu);
+  protected Promise<Integer> launchAsyncActivity(final WorkUnitClaimCheck wu, final Properties props) {


Do we plan to add unit tests for this?

Unit tests for older temporal classes needs to be done separately as it will required dedicated effort for this which is beyond the scope of this PR

...rc/main/java/org/apache/gobblin/temporal/ddm/workflow/impl/ProcessWorkUnitsWorkflowImpl.java

.../java/org/apache/gobblin/temporal/util/nesting/workflow/AbstractNestingExecWorkflowImpl.java

vsinghal85 · 2025-02-04T05:01:00Z

As discussed offline, please add the testing details

codecov-commenter · 2025-02-04T05:13:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.75%. Comparing base (a0cef28) to head (ab3789a).
Report is 2 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (a0cef28) and HEAD (ab3789a). Click for more details.

HEAD has 2 uploads less than BASE

Flag BASE (a0cef28) HEAD (ab3789a)

3 1

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #4093       +/-   ##
=============================================
- Coverage     49.03%   38.75%   -10.29%     
+ Complexity    10008     1599     -8409     
=============================================
  Files          1895      388     -1507     
  Lines         73612    16016    -57596     
  Branches       8188     1588     -6600     
=============================================
- Hits          36097     6207    -29890     
+ Misses        34280     9311    -24969     
+ Partials       3235      498     -2737

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

Copilot

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;

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

khandelwal-prateek · 2025-02-07T07:49:32Z

...al/src/main/java/org/apache/gobblin/temporal/ddm/activity/ActivityConfigurationStrategy.java

+  int DEFAULT_GENERATE_WORKUNITS_ACTIVITY_STARTTOCLOSE_TIMEOUT_MINUTES = 120;
+  int DEFAULT_RECOMMEND_SCALING_ACTIVITY_STARTTOCLOSE_TIMEOUT_MINUTES = 5;
+  int DEFAULT_DELETE_WORK_DIRS_ACTIVITY_STARTTOCLOSE_TIMEOUT_MINUTES = 10;
+  int DEFAULT_PROCESS_WORKUNIT_ACTIVITY_STARTTOCLOSE_TIMEOUT_MINUTES = 180;
+  int DEFAULT_COMMIT_ACTIVITY_STARTTOCLOSE_TIMEOUT_MINUTES = 180;


let's update the default numbers to a higher value for now till we have better visibility and tuning in place. Also, we already see flows which are running close to these number and timing out those isn't correct eg. GENERATE_WORKUNITS=4h, PROCESS_WORKUNIT=6h, COMMIT=4h

do we know how much max time max takes/can take and what is the factor that increases the time, if no, let's also increase DELETE_WORK_DIRS/ RECOMMEND_SCALING time

I have updated to use a single DEFAULT time for all activity type to simplify the code

khandelwal-prateek · 2025-02-07T07:50:42Z

...al/src/main/java/org/apache/gobblin/temporal/ddm/activity/ActivityConfigurationStrategy.java

+public interface ActivityConfigurationStrategy {
+  /** Default start to close timeout duration for any activity if not specified. */
+  Duration defaultStartToCloseTimeout = Duration.ofMinutes(180);
+  int DEFAULT_GENERATE_WORKUNITS_ACTIVITY_STARTTOCLOSE_TIMEOUT_MINUTES = 120;


do we have numbers currently, how long can it take for each activity, ideally we want this number to be higher than 100th %ile of all executions that are running and also having scope for what we support(max data copy size - something that we will have as part of our SLAs for users)

Used 360 as default timeout for all activity

...al/src/main/java/org/apache/gobblin/temporal/ddm/activity/ActivityConfigurationStrategy.java

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/activity/ActivityType.java

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

Blazer-007 added 2 commits January 23, 2025 10:34

initial changes for adding timeout strategy

2e0f63d

added java docs

40ec90e

Blazer-007 changed the title ~~[GOBBLIN-XXX] Implement ActivityTimeoutStrategy for Temporal Activities~~ [GOBBLIN-2190] Implement ActivityTimeoutStrategy for Temporal Activities Jan 23, 2025

added tests

d231c4e

Blazer-007 marked this pull request as ready for review January 23, 2025 12:34

changed interface name

a3cb881

Blazer-007 changed the title ~~[GOBBLIN-2190] Implement ActivityTimeoutStrategy for Temporal Activities~~ [GOBBLIN-2190] Implement ActivityConfigurationStrategy for Temporal Activities Jan 31, 2025

vsinghal85 reviewed Feb 3, 2025

View reviewed changes

corrected javadoc

ab3789a

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

Copilot AI reviewed Feb 6, 2025

View reviewed changes

gobblin-temporal/src/main/java/org/apache/gobblin/temporal/ddm/util/TemporalActivityUtils.java Outdated Show resolved Hide resolved

khandelwal-prateek reviewed Feb 7, 2025

View reviewed changes

Blazer-007 and others added 5 commits February 18, 2025 22:57

remove strategy class and modify enum itself

c043879

passing only temporal prefix props to childworkflows

d7e4e76

add activity heartbeat

a5efc3b

Merge branch 'master' into virai_make_timeout_configurable

6e87ca6

fix checkstyle

ef1deac

Blazer-007 changed the title ~~[GOBBLIN-2190] Implement ActivityConfigurationStrategy for Temporal Activities~~ [GOBBLIN-2190] Implement ActivityType & add HeartBeat for Temporal Activities Feb 19, 2025

added tests for activity type

9639a2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-2190] Implement ActivityType & add HeartBeat for Temporal Activities #4093

[GOBBLIN-2190] Implement ActivityType & add HeartBeat for Temporal Activities #4093

Blazer-007 commented Jan 23, 2025 •

edited

Loading

vsinghal85 Feb 3, 2025

Blazer-007 Feb 3, 2025

vsinghal85 commented Feb 4, 2025

codecov-commenter commented Feb 4, 2025

Copilot AI left a comment

khandelwal-prateek Feb 7, 2025

Blazer-007 Feb 20, 2025

khandelwal-prateek Feb 7, 2025

Blazer-007 Feb 20, 2025

[GOBBLIN-2190] Implement ActivityType & add HeartBeat for Temporal Activities #4093

Are you sure you want to change the base?

[GOBBLIN-2190] Implement ActivityType & add HeartBeat for Temporal Activities #4093

Conversation

Blazer-007 commented Jan 23, 2025 • edited Loading

JIRA

Description

Tests

Commits

vsinghal85 Feb 3, 2025

Choose a reason for hiding this comment

Blazer-007 Feb 3, 2025

Choose a reason for hiding this comment

vsinghal85 commented Feb 4, 2025

codecov-commenter commented Feb 4, 2025

Codecov Report

Copilot AI left a comment

Choose a reason for hiding this comment

khandelwal-prateek Feb 7, 2025

Choose a reason for hiding this comment

Blazer-007 Feb 20, 2025

Choose a reason for hiding this comment

khandelwal-prateek Feb 7, 2025

Choose a reason for hiding this comment

Blazer-007 Feb 20, 2025

Choose a reason for hiding this comment

Blazer-007 commented Jan 23, 2025 •

edited

Loading