[GOBBLIN-1983] Remove Optionals to make DagManager, EventSubmitter, and TopologyCatalog required for GaaS operation #3855

arjun4084346 · 2024-01-10T23:03:14Z

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

Description

Here are some details about my PR, including screenshots (if applicable):
fields isInstrumentationEnabled needs to be always enabled and eventSubmitter should always be present for GaaS to function. Removing Optional from them.

Tests

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

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-01-10T23:24:19Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (22248e4) 47.56% compared to head (f7f7cdf) 45.19%.

Files	Patch %	Lines
...in/service/modules/orchestration/Orchestrator.java	72.72%	5 Missing and 1 partial ⚠️
...modules/utils/FlowCompilationValidationHelper.java	0.00%	6 Missing ⚠️
...in/service/modules/core/GobblinServiceManager.java	88.88%	1 Missing ⚠️
...service/modules/orchestration/DagManagerUtils.java	50.00%	0 Missing and 1 partial ⚠️
.../modules/scheduler/GobblinServiceJobScheduler.java	0.00%	1 Missing ⚠️
.../service/monitoring/LocalFsJobStatusRetriever.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3855      +/-   ##
============================================
- Coverage     47.56%   45.19%   -2.38%     
+ Complexity    11068     5186    -5882     
============================================
  Files          2160     1088    -1072     
  Lines         85563    44311   -41252     
  Branches       9509     4856    -4653     
============================================
- Hits          40700    20026   -20674     
+ Misses        41154    22409   -18745     
+ Partials       3709     1876    -1833

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

umustafi · 2024-01-11T01:08:54Z

gobblin-core-base/src/main/java/org/apache/gobblin/instrumented/Instrumented.java

    markMeter(meter, 1);
  }

+  public static void markMeter(Meter meter) {


where is this being used?

my related Q is whether we still require the Optional version or could replace them w/ this one

There are many non-gaas callers to this method with Optional parameter, which may be providing parameter that is truly optional.

arjun4084346 · 2024-01-11T01:34:35Z

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

-        if (compiledDag == null || compiledDag.isEmpty()) {
+        if (compiledDag.isEmpty()) {
          FlowCompilationValidationHelper.populateFlowCompilationFailedEventMessage(eventSubmitter, spec, flowMetadata);
          Instrumented.markMeter(this.flowOrchestrationFailedMeter);


It is used here, the parameter changed from Optional to X

phet

nice cleanup, arjun!

phet · 2024-01-11T01:34:09Z

gobblin-core-base/src/main/java/org/apache/gobblin/instrumented/Instrumented.java

    markMeter(meter, 1);
  }

+  public static void markMeter(Meter meter) {


my related Q is whether we still require the Optional version or could replace them w/ this one

phet · 2024-01-11T01:36:48Z

gobblin-core-base/src/main/java/org/apache/gobblin/instrumented/Instrumented.java

 import com.google.common.collect.Lists;
 import com.google.common.io.Closer;

+import javax.annotation.Nonnull;


when I see:

Import order: java, org, com, gobblin.

here - https://gobblin.apache.org/docs/developer-guide/CodingStyle/

I presumed javax would follow java, not pop up between com and gobblin. are you sure this is supposed to move here?

I do not do any import changes manually, intelliJ does it, hopefully based on the provided codestyle

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

phet · 2024-01-11T01:51:28Z

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

   * @param flowSpec
   * @param optionalFlowExecutionId for scheduled (non-ad-hoc) flows, to pass the ID "laundered" via the DB;
-   *                                see: {@link MysqlMultiActiveLeaseArbiter javadoc section titled
+   *                                see: {@link org.apache.gobblin.runtime.api.MysqlMultiActiveLeaseArbiter javadoc section titled


was this necessary? (I didn't see you remove an import)

just to fix javadoc

phet

whew, what a relief to pare down all this out-of-date and out-of-use code - very nice!

remove from the fields that are actually mandatory

81757ce

arjun4084346 force-pushed the removeOptional branch from 6cf5575 to 81757ce Compare January 10, 2024 23:10

arjun4084346 changed the title ~~remove from the fields that are actually mandatory~~ [GOBBLIN-1983] remove from the fields that are actually mandatory Jan 10, 2024

umustafi reviewed Jan 11, 2024

View reviewed changes

arjun4084346 commented Jan 11, 2024

View reviewed changes

umustafi approved these changes Jan 11, 2024

View reviewed changes

phet reviewed Jan 11, 2024

View reviewed changes

arjun4084346 force-pushed the removeOptional branch from 7110bc1 to e49f7ff Compare January 11, 2024 20:02

remove optionality of dag manager and topology catalog

f7f7cdf

arjun4084346 force-pushed the removeOptional branch from e49f7ff to f7f7cdf Compare January 11, 2024 21:16

phet approved these changes Jan 12, 2024

View reviewed changes

arjun4084346 changed the title ~~[GOBBLIN-1983] remove from the fields that are actually mandatory~~ [GOBBLIN-1983] Remove Optionals to make DagManager, EventSubmitter, and TopologyCatalog required for GaaS operation Jan 12, 2024

arjun4084346 merged commit be12fd0 into apache:master Jan 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[GOBBLIN-1983] Remove Optionals to make DagManager, EventSubmitter, and TopologyCatalog required for GaaS operation #3855

[GOBBLIN-1983] Remove Optionals to make DagManager, EventSubmitter, and TopologyCatalog required for GaaS operation #3855

Uh oh!

arjun4084346 commented Jan 10, 2024 •

edited

Loading

Uh oh!

codecov-commenter commented Jan 10, 2024 •

edited

Loading

Uh oh!

umustafi Jan 11, 2024

Uh oh!

phet Jan 11, 2024

Uh oh!

arjun4084346 Jan 11, 2024

Uh oh!

arjun4084346 Jan 11, 2024

Uh oh!

phet left a comment

Uh oh!

phet Jan 11, 2024

Uh oh!

phet Jan 11, 2024 •

edited

Loading

Uh oh!

arjun4084346 Jan 11, 2024

Uh oh!

Uh oh!

phet Jan 11, 2024 •

edited

Loading

Uh oh!

arjun4084346 Jan 11, 2024

Uh oh!

phet left a comment

Uh oh!

Uh oh!

[GOBBLIN-1983] Remove Optionals to make DagManager, EventSubmitter, and TopologyCatalog required for GaaS operation #3855

[GOBBLIN-1983] Remove Optionals to make DagManager, EventSubmitter, and TopologyCatalog required for GaaS operation #3855

Uh oh!

Conversation

arjun4084346 commented Jan 10, 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 Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

umustafi Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

phet Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

arjun4084346 Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

arjun4084346 Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

phet left a comment

Choose a reason for hiding this comment

Uh oh!

phet Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

phet Jan 11, 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 Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phet Jan 11, 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 Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

phet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arjun4084346 commented Jan 10, 2024 •

edited

Loading

codecov-commenter commented Jan 10, 2024 •

edited

Loading

phet Jan 11, 2024 •

edited

Loading

phet Jan 11, 2024 •

edited

Loading