[GOBBLIN-2136] Byebye dag manager #4031

arjun4084346 · 2024-08-16T03:34:35Z

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-2136

Description

Here are some details about my PR, including screenshots (if applicable):

Now when we have developed the new GaaS architecture based on DagProcessingEngine, we do not need DagManager and related classes. This also enabled GaaS to work in a multi leader mode, so there is no need for the code that was there for standby hosts.

a)
Removed the code path for GOBBLIN_SERVICE_FLOW_CATALOG_LOCAL_COMMIT = false
Removed the code path for FORCE_LEADER = true
Removed the code path for MULTI_ACTIVE_EXECUTION_ENABLED = false
Removed the code path for MULTI_ACTIVE_SCHEDULER_ENABLED = false
Removed the code path for GOBBLIN_SERVICE_WARM_STANDBY_ENABLED_KEY = false
Removed the code path for DAG_PROC_ENGINE_ENABLED = false

b)
Removed DagManager

c)
Removed Optional from DagManagement, DagTaskStream, DagProcFactory, DagProcessingEngine, DagManagementStateStore, FlowLaunchHandler, DagActionReminderScheduler

d)
Codestyle changes suggested by IntelliJ

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
no code logic change, only updated the existing tests

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"

codecov-commenter · 2024-08-18T19:16:33Z

Codecov Report

Attention: Patch coverage is 68.04124% with 93 lines in your changes missing coverage. Please review.

Project coverage is 48.25%. Comparing base (444f266) to head (349f2b9).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
.../modules/scheduler/GobblinServiceJobScheduler.java	55.81%	19 Missing ⚠️
...les/orchestration/DagManagementTaskStreamImpl.java	10.52%	17 Missing ⚠️
...e/modules/orchestration/MysqlUserQuotaManager.java	0.00%	14 Missing ⚠️
...rvice/modules/orchestration/DagManagerMetrics.java	44.44%	5 Missing ⚠️
...in/service/modules/core/GobblinServiceManager.java	71.42%	4 Missing ⚠️
...odules/orchestration/InMemoryUserQuotaManager.java	78.94%	2 Missing and 2 partials ⚠️
...in/service/modules/orchestration/Orchestrator.java	69.23%	2 Missing and 2 partials ⚠️
...rvice/monitoring/KafkaJobStatusMonitorFactory.java	0.00%	4 Missing ⚠️
...ache/gobblin/runtime/spec_catalog/FlowCatalog.java	62.50%	2 Missing and 1 partial ⚠️
...obblin/service/modules/orchestration/DagUtils.java	62.50%	3 Missing ⚠️
... and 11 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4031      +/-   ##
============================================
+ Coverage     38.79%   48.25%   +9.45%     
- Complexity     1599     9111    +7512     
============================================
  Files           388     1778    +1390     
  Lines         15998    68211   +52213     
  Branches       1585     7315    +5730     
============================================
+ Hits           6207    32913   +26706     
- Misses         9293    32513   +23220     
- Partials        498     2785    +2287

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

phet

this PR is a monster! (hard to believe I'm not even 3/4 done yet)

maybe github will give you a badge for the largest PR. I sure hope I get one for reviewing it ;p

that aside, this looks like great work, which I'm eager to incorporate into HEAD. I'll return to finish my first pass ASAP.

my biggest concern is you've clubbed together removing the DagMgr w/ reworking the REST APIs. if we somehow needed to revert one or the other change, they suddenly become inseparable.

changing public APIs like the REST one often requires care, so I'd definitely isolate that one. it will help maintainers as well as end-users to grasp when that change landed, which is harder to do if the commit message is this other one about removing the DM

phet · 2024-08-29T20:06:18Z

gobblin-api/src/main/java/org/apache/gobblin/service/ServiceConfigKeys.java

+  public static final String JOB_START_SLA_TIME = GOBBLIN_SERVICE_DAG_PROCESSING_ENGINE_PREFIX + ConfigurationKeys.GOBBLIN_JOB_START_SLA_TIME;
+  public static final String JOB_START_SLA_UNITS = GOBBLIN_SERVICE_DAG_PROCESSING_ENGINE_PREFIX + ConfigurationKeys.GOBBLIN_JOB_START_SLA_TIME_UNIT;
+  public static final long DEFAULT_FLOW_SLA_MILLIS = TimeUnit.HOURS.toMillis(24);


three thoughts here:

previously the property included the .dagManager segment:

// Default job start SLA time if configured, measured in minutes. Default is 10 minutes // todo - rename "sla" -> "deadline", and move them to DagProcUtils public static final String JOB_START_SLA_TIME = DAG_MANAGER_PREFIX + ConfigurationKeys.GOBBLIN_JOB_START_SLA_TIME;

should we continue to accept that legacy prop name so existing flow defs remain b/w compat?

as we introduce a new prop name, let's take the opportunity to align naming and semantics by calling it start.deadline, rather than SLA

given that .dagProcessingEngine is as much impl. detail as .dagManager, it may not belong in the customer-facing name.

gobblin-runtime/src/main/java/org/apache/gobblin/runtime/spec_catalog/FlowCatalog.java

phet · 2024-08-29T20:32:48Z

...service/src/main/java/org/apache/gobblin/service/modules/flow/BaseFlowToJobSpecCompiler.java

@@ -74,7 +74,7 @@ public abstract class BaseFlowToJobSpecCompiler implements SpecCompiler {
  // to these data structures.
  @Getter
  @Setter
-  protected final Map<URI, TopologySpec> topologySpecMap;
+  protected Map<URI, TopologySpec> topologySpecMap;


I didn't notice why this wouldn't still be final. is a derived class assigning to it?

yea it should be.... i thought if there is a @Setter it should not be final. removed the @Setter instead

gobblin-service/src/main/java/org/apache/gobblin/service/modules/flowgraph/Dag.java

phet · 2024-08-29T20:38:51Z

...src/main/java/org/apache/gobblin/service/modules/orchestration/AbstractUserQuotaManager.java

+  public static final String PER_USER_QUOTA = ServiceConfigKeys.GOBBLIN_SERVICE_DAG_PROCESSING_ENGINE_PREFIX + "perUserQuota";
+  public static final String PER_FLOWGROUP_QUOTA = ServiceConfigKeys.GOBBLIN_SERVICE_DAG_PROCESSING_ENGINE_PREFIX + "perFlowGroupQuota";
+  public static final String USER_JOB_QUOTA_KEY = ServiceConfigKeys.GOBBLIN_SERVICE_DAG_PROCESSING_ENGINE_PREFIX + "defaultJobQuota";


again, should we honor the old prop names for b/w compat?

also, when we devise a new name to deprecate the old one, let's avoid embedding impl-specific identifiers in what should otherwise be a user-level config option w/ clear and steady semantics

at least in linkedin no one is using them, so i would prefer to remove them. and if someone outside them is using them, we would not know we can make the switch

gobblin-service/src/test/java/org/apache/gobblin/service/monitoring/GitConfigMonitorTest.java

...lin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagTestUtils.java

phet · 2024-08-29T22:41:18Z

gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagFlowTest.java

is this the right class? isn't this more of a "JobSpecTest"?

moved to DagUtilsTest

...lin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/Orchestrator.java

.../main/java/org/apache/gobblin/service/modules/orchestration/DagManagementTaskStreamImpl.java

gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java

gobblin-api/src/main/java/org/apache/gobblin/service/ServiceConfigKeys.java

.../main/java/org/apache/gobblin/service/modules/orchestration/DagManagementTaskStreamImpl.java

...ervice/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManagerMetrics.java

...main/java/org/apache/gobblin/service/modules/orchestration/MySqlDagManagementStateStore.java

...ervice/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/DagProcUtils.java

...va/org/apache/gobblin/service/modules/orchestration/proc/EnforceJobStartDeadlineDagProc.java

...lin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagUtilsTest.java

phet

great work so far! this looks very close

gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java

gobblin-api/src/main/java/org/apache/gobblin/service/ServiceConfigKeys.java

...lin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java

.../src/main/java/org/apache/gobblin/service/modules/utils/FlowCompilationValidationHelper.java

...e/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java

...ain/java/org/apache/gobblin/service/monitoring/DagManagementDagActionStoreChangeMonitor.java

...lin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/Orchestrator.java

gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java

...service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java

phet

incredible work here on this enormous cleanup, arjun!

arjun4084346 force-pushed the byebyeDagManager branch 9 times, most recently from 1e9b843 to 4f71190 Compare August 17, 2024 06:14

arjun4084346 force-pushed the byebyeDagManager branch 6 times, most recently from f1c070f to 30d9638 Compare August 20, 2024 00:18

arjun4084346 changed the title ~~[draft] Byebye dag manager~~ [GOBBLIN-2136] Byebye dag manager Aug 20, 2024

arjun4084346 force-pushed the byebyeDagManager branch 6 times, most recently from 4462811 to bc27263 Compare August 26, 2024 23:05

arjun4084346 force-pushed the byebyeDagManager branch from d257fc4 to d775509 Compare August 27, 2024 04:37

phet reviewed Aug 29, 2024

View reviewed changes

arjun4084346 force-pushed the byebyeDagManager branch 5 times, most recently from 5194b07 to a8c4203 Compare August 31, 2024 03:49

arjun4084346 force-pushed the byebyeDagManager branch from a8c4203 to 9c99c27 Compare September 4, 2024 18:06

phet reviewed Sep 4, 2024

View reviewed changes

arjun4084346 added 8 commits September 4, 2024 18:57

removed DagManager

4584a02

remove flowconfigsv1

6664a0b

merge conflicts

9f1cc4b

address review comments

5468829

address review comments

3ec405f

address review comments

5d1594d

address review comments

cb81632

address review comments

65a1e05

arjun4084346 force-pushed the byebyeDagManager branch from 7eac51e to 65a1e05 Compare September 5, 2024 03:45

revert back some config name changes

349f2b9

arjun4084346 force-pushed the byebyeDagManager branch from fd652f0 to 349f2b9 Compare September 5, 2024 04:26

address review comments

1a8afbf

phet reviewed Sep 5, 2024

View reviewed changes

arjun4084346 added 2 commits September 5, 2024 11:39

address review comments

87e4cd2

address review comments

325a242

phet approved these changes Sep 5, 2024

View reviewed changes

arjun4084346 merged commit 45ad13e into apache:master Sep 7, 2024
6 checks passed

arjun4084346 mentioned this pull request Sep 9, 2024

[GOBBLIN-2136] fixing an issue of not able to connect to db #4051

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-2136] Byebye dag manager #4031

[GOBBLIN-2136] Byebye dag manager #4031

arjun4084346 commented Aug 16, 2024 •

edited

Loading

codecov-commenter commented Aug 18, 2024 •

edited

Loading

phet left a comment

phet Aug 29, 2024

phet Aug 29, 2024

arjun4084346 Aug 30, 2024

phet Aug 29, 2024

arjun4084346 Aug 30, 2024

phet Aug 29, 2024

arjun4084346 Aug 30, 2024

phet left a comment

phet left a comment

[GOBBLIN-2136] Byebye dag manager #4031

[GOBBLIN-2136] Byebye dag manager #4031

Conversation

arjun4084346 commented Aug 16, 2024 • edited Loading

JIRA

Description

Tests

Commits

codecov-commenter commented Aug 18, 2024 • edited Loading

Codecov Report

phet left a comment

Choose a reason for hiding this comment

phet Aug 29, 2024

Choose a reason for hiding this comment

phet Aug 29, 2024

Choose a reason for hiding this comment

arjun4084346 Aug 30, 2024

Choose a reason for hiding this comment

phet Aug 29, 2024

Choose a reason for hiding this comment

arjun4084346 Aug 30, 2024

Choose a reason for hiding this comment

phet Aug 29, 2024

Choose a reason for hiding this comment

arjun4084346 Aug 30, 2024

Choose a reason for hiding this comment

phet left a comment

Choose a reason for hiding this comment

phet left a comment

Choose a reason for hiding this comment

arjun4084346 commented Aug 16, 2024 •

edited

Loading

codecov-commenter commented Aug 18, 2024 •

edited

Loading