[GOBBLIN-2013] update guice initialization for 'DagProcEngine enabled' and related classes by arjun4084346 · Pull Request #3892 · apache/gobblin

arjun4084346 · 2024-03-09T22:58:54Z

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

Description

Here are some details about my PR, including screenshots (if applicable):
creating instances of new new class created in PR [GOBBLIN-1910] Add DagProcEngine, DagManagement, DagTask, DagProc, and other abstractions for refactored DAG management #3858

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
updated 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-03-10T01:43:43Z

Codecov Report

Attention: Patch coverage is 85.36585% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 46.55%. Comparing base (02eca4f) to head (6fc087e).

Files	Patch %	Lines
...ervice/modules/core/GobblinServiceGuiceModule.java	69.23%	4 Missing ⚠️
...modules/utils/FlowCompilationValidationHelper.java	86.66%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3892      +/-   ##
============================================
+ Coverage     46.53%   46.55%   +0.02%     
- Complexity    11149    11158       +9     
============================================
  Files          2231     2231              
  Lines         87881    87911      +30     
  Branches       9632     9633       +1     
============================================
+ Hits          40893    40929      +36     
+ Misses        43288    43287       -1     
+ Partials       3700     3695       -5

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

umustafi · 2024-03-11T22:03:19Z

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

      metricContext.register(this.totalAddSpecTimeNanos);
      metricContext.register(this.numJobsScheduledDuringStartup);
    }
+    this.dagProcessingEngineEnabled = ConfigUtils.getBoolean(config, ServiceConfigKeys.DAG_PROCESSING_ENGINE_ENABLED, false);


u can also add this as an injected param. up to u (look at line 183)

umustafi · 2024-03-11T22:07:20Z

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

+      if (this.dagProcessingEngineEnabled) {
+        Config flowConfig = flowSpec.getConfig();
+        String flowGroup = flowConfig.getString(ConfigurationKeys.FLOW_GROUP_KEY);
+        String flowName = flowConfig.getString(ConfigurationKeys.FLOW_NAME_KEY);
+        String flowExecutionId = String.valueOf(FlowUtils.getOrCreateFlowExecutionId(flowSpec));
+        DagActionStore.DagAction dagAction =
+            new DagActionStore.DagAction(flowGroup, flowName, flowExecutionId, DagActionStore.FlowActionType.LAUNCH);
+        this.dagManagement.addDagAction(dagAction);
+      } else {
+        this.orchestrator.orchestrate(flowSpec, jobProps, Long.parseLong(triggerTimestampMillis), isReminderEvent);
+      }


This is not the correct place to add dag action to the stream. Orchestrate is called in response to scheduler and is where scheduler lease arbitration will happen. As a result of it, the launch dagAction will be committed to dagActionStore -> read by changeMonitor -> then forwarded to the dagManager. It's at the last step that we should add the launch task to the stream (via dagManagement) instead of pass to old dagManager. This change should be in DagProcEnabledDagActionStoreChangeMonitor

Also wherever your enable this let's add a test

umustafi · 2024-03-11T23:10:56Z

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

      Orchestrator orchestrator, SchedulerService schedulerService, Optional<UserQuotaManager> quotaManager, Optional<Logger> log,
      @Named(InjectionNames.WARM_STANDBY_ENABLED) boolean isWarmStandbyEnabled,
-      Optional<FlowTriggerHandler> flowTriggerHandler) throws Exception {
+      Optional<FlowTriggerHandler> flowTriggerHandler, DagManagement dagManagement,


it's also clearer into to have an optional DagManagement object that is present only when the engine is enabled but it's open to debate

Yea, I actually wanted to create those objects regardless of the enabled flag so at least the object creation can be tested in prod.

I'd whole-heartedly urge us to ONLY create required instances. otherwise a not-yet-ready-for-prod instance that fails creation can bring down the entire application. best practice is to guard significant new/reworked functionality behind config, and this includes initialization

phet

I'm confused: how does this PR relate to #3893 (review) ?

there seems to be some, but not all overlap. if two PRs relate to one another, and have any overlap at all, please have at least one mention the other in the PR description to clarify their relationship.

separately, but also critical: this PR title really brings very little clarity and seems insufficient (eventually) for a commit message

phet · 2024-03-12T03:26:29Z

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

      Orchestrator orchestrator, SchedulerService schedulerService, Optional<UserQuotaManager> quotaManager, Optional<Logger> log,
      @Named(InjectionNames.WARM_STANDBY_ENABLED) boolean isWarmStandbyEnabled,
-      Optional<FlowTriggerHandler> flowTriggerHandler) throws Exception {
+      Optional<FlowTriggerHandler> flowTriggerHandler, DagManagement dagManagement,


I'd whole-heartedly urge us to ONLY create required instances. otherwise a not-yet-ready-for-prod instance that fails creation can bring down the entire application. best practice is to guard significant new/reworked functionality behind config, and this includes initialization

phet · 2024-03-12T03:29:07Z

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

  protected final Optional<FlowTriggerHandler> flowTriggerHandler;
  @Getter
-  protected final Map<String, Spec> scheduledFlowSpecs;
+  protected final Map<String, FlowSpec> scheduledFlowSpecs;


NBD, this may be for the best... but I can't seem to figure out: where is the initial motivation coming in? e.g. I was looking for a changed method signature, but didn't notice one. is it entirely a preference to change the type of these various members?

(again, I'm not calling into question the decision to change, more wanting to understand motivation for the decision.)

I believe the initial commit was too big and some improvements were missed out

aah, ok, I see - so essentially just following more widely the specific-derived-class (FlowSpec) typing instituted therein.

(I like the increased clarity)

phet

this is a much better PR title... but the context behind the change still may not be readily apparent for anyone reading such a commit message in 2-3 months.

how about: "update guice initialization for 'DagProcEngine enabled'"?

phet · 2024-03-12T05:05:53Z

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

  @Getter
  private final SharedFlowMetricsSingleton sharedFlowMetricsSingleton;

-  private final ClassAliasResolver<SpecCompiler> aliasResolver;


good call to rescope as method level!

phet · 2024-03-12T05:07:04Z

...rvice/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/LaunchDagProc.java

+    ContextAwareGauge<Long> orchestrationDelayMetric = metricContext.newContextAwareGauge
        (ServiceMetricNames.FLOW_ORCHESTRATION_DELAY, orchestrationDelayCounter::get);
-    this.metricContext.register(orchestrationDelayMetric);
+    metricContext.register(orchestrationDelayMetric);


why change how it's referenced... isn't metricContext still an instance member?

actually it is not, it is static

phet · 2024-03-12T05:11:32Z

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

+      String specCompilerClassName = ServiceConfigKeys.DEFAULT_GOBBLIN_SERVICE_FLOWCOMPILER_CLASS;
+      if (config.hasPath(ServiceConfigKeys.GOBBLIN_SERVICE_FLOWCOMPILER_CLASS_KEY)) {
+        specCompilerClassName = config.getString(ServiceConfigKeys.GOBBLIN_SERVICE_FLOWCOMPILER_CLASS_KEY);


isn't there a method in ConfigUtils (or similar utility) that we could leverage to provide such fallback to a default?

I do not think so.

what about this one:

String ConfigUtils::getString(Config config, String path, String def)

?

oh i misread your ask.

phet · 2024-03-12T05:16:41Z

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

  protected final Optional<FlowTriggerHandler> flowTriggerHandler;
  @Getter
-  protected final Map<String, Spec> scheduledFlowSpecs;
+  protected final Map<String, FlowSpec> scheduledFlowSpecs;


aah, ok, I see - so essentially just following more widely the specific-derived-class (FlowSpec) typing instituted therein.

(I like the increased clarity)

phet

nice work turning this one around in such short order!

arjun4084346 force-pushed the testdag branch 3 times, most recently from c51c82d to 5d51351 Compare March 10, 2024 01:31

arjun4084346 force-pushed the testdag branch 2 times, most recently from 8ce2784 to 2505b62 Compare March 11, 2024 05:23

update guice module

1123345

arjun4084346 force-pushed the testdag branch from 3285d7c to 1123345 Compare March 11, 2024 20:11

arjun4084346 changed the title ~~update guice module~~ [GOBBLIN-2013] update guice module Mar 11, 2024

umustafi reviewed Mar 11, 2024

View reviewed changes

address review comments

f285a91

umustafi approved these changes Mar 11, 2024

View reviewed changes

phet reviewed Mar 12, 2024

View reviewed changes

arjun4084346 force-pushed the testdag branch from ece4c88 to 953e733 Compare March 12, 2024 04:15

arjun4084346 changed the title ~~[GOBBLIN-2013] update guice module~~ [GOBBLIN-2013] create instances through guice for new classes created in PR #3858 Mar 12, 2024

address review comments

76aae7c

arjun4084346 force-pushed the testdag branch from 953e733 to 76aae7c Compare March 12, 2024 04:34

arjun4084346 closed this Mar 12, 2024

arjun4084346 reopened this Mar 12, 2024

arjun4084346 closed this Mar 12, 2024

arjun4084346 reopened this Mar 12, 2024

phet reviewed Mar 12, 2024

View reviewed changes

arjun4084346 changed the title ~~[GOBBLIN-2013] create instances through guice for new classes created in PR #3858~~ [GOBBLIN-2013] update guice initialization for 'DagProcEngine enabled' and related classes Mar 12, 2024

address review comment

6fc087e

phet approved these changes Mar 12, 2024

View reviewed changes

arjun4084346 merged commit 2217205 into apache:master Mar 12, 2024

Conversation

arjun4084346 commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Description

Tests

Commits

Uh oh!

codecov-commenter commented Mar 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phet Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arjun4084346 Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phet Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phet Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arjun4084346 commented Mar 9, 2024 •

edited

Loading

codecov-commenter commented Mar 10, 2024 •

edited

Loading

phet left a comment •

edited

Loading

phet Mar 12, 2024 •

edited

Loading

arjun4084346 Mar 12, 2024 •

edited

Loading

phet Mar 12, 2024 •

edited

Loading

phet left a comment •

edited

Loading

phet Mar 12, 2024 •

edited

Loading