[GOBBLIN-2136] consolidate various resource handlers for flowconfigs #4041

arjun4084346 · 2024-08-29T22:27:02Z

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):
a)
Merged FlowConfigsResourceHandler, FlowConfigV2ResourceLocalHandler, FlowConfigResourceLocalHandler, FlowConfigsV2ResourceHandler, GobblinServiceFlowConfigResourceHandler, GobblinServiceFlowConfigV2ResourceHandler, GobblinServiceFlowConfigV2ResourceHandlerWithWarmStandby into FlowConfigsResourceHandlerFlowConfigsV2ResourceHandler

Merged FlowExecutionResourceLocalHandler, GobblinServiceFlowExecutionResourceHandler, GobblinServiceFlowExecutionResourceHandlerWithWarmStandby into FlowExecutionResourceHandler

Merged FlowConfigClient into FlowConfigV2Client along with their test classes

b)
Removed Helix and EventBus related code from GaaS

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
no functiotnality 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-09-08T01:46:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.28%. Comparing base (45ad13e) to head (6ef073a).
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4041      +/-   ##
============================================
+ Coverage     45.12%   50.28%   +5.16%     
- Complexity     3199     5912    +2713     
============================================
  Files           705     1073     +368     
  Lines         26949    41240   +14291     
  Branches       2680     4633    +1953     
============================================
+ Hits          12160    20739    +8579     
- Misses        13781    18724    +4943     
- Partials       1008     1777     +769

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

...n-flow-config-service-api/src/main/idl/org.apache.gobblin.service.flowstatuses.restspec.json

...w-config-service-api/src/main/snapshot/org.apache.gobblin.service.flowstatuses.snapshot.json

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

phet · 2024-09-09T17:38:44Z

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

  protected void assignTopicPartitions() {
    // Expects underlying consumer to handle initializing partitions and offset for the topic -
    // subscribe to all partitions from latest offset


I'm a bit fuzzy on this (and wondering whether it should be an abstract method).

if the empty but concrete impl is actually what we want, is it possible to identify the specific method involved in the "underlying consumer"?

I think all the implementations of HLC except this one needs this method and this is implemented in HLC.

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

phet · 2024-09-09T17:49:12Z

...ce/src/main/java/org/apache/gobblin/service/modules/restli/FlowExecutionResourceHandler.java

+
+
+@Slf4j
+public class FlowExecutionResourceHandler implements FlowExecutionResourceHandlerInterface {


javadoc?

also, the diff isn't showing this as a rename, but is that what it is?

added javadoc. github issues

...vice-server/src/test/java/org/apache/gobblin/service/FlowConfigResourceLocalHandlerTest.java

...e-server/src/test/java/org/apache/gobblin/service/FlowExecutionResourceLocalHandlerTest.java

phet · 2024-09-09T20:51:21Z

...er/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java

 @Slf4j
-public class FlowConfigResourceLocalHandler implements FlowConfigsResourceHandler {
+public class FlowConfigsV2ResourceHandler {


why not implement a FlowConfigsV2ResourceHandlerInterface, like flowExecutions does?

// Unlike FlowConfigsV2ResourceHandler, FlowExecutionResourceHandlerInterface is an interface rather than a class because it's implementation needs // classes from gobblin-service module, and adding gobblin-service as a dependency will cause circular dependency

now i am thinking to move FlowExecutionResource that needs it to gobblin-service module, so I can get rid of FlowExecutionResourceHandlerInterface. wdyt?

gobblin-service/src/test/java/org/apache/gobblin/service/FlowExecutionTest.java

...ice-server/src/main/java/org/apache/gobblin/service/FlowConfigsResourceHandlerInterface.java

...e-server/src/main/java/org/apache/gobblin/service/FlowExecutionResourceHandlerInterface.java

phet · 2024-09-10T02:13:04Z

...er/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java

    // We have modifiedWatermark here to avoid update config happens at the same time on different hosts overwrite each other
    // timestamp here will be treated as largest modifiedWatermark that we can update
    long version = System.currentTimeMillis() / 1000;
    return updateFlowConfig(flowId, flowConfig, version);
  }

-  public UpdateResponse updateFlowConfig(FlowId flowId,
-      FlowConfig flowConfig, long modifiedWatermark) throws FlowConfigLoggedException {
+  private UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig, long modifiedWatermark) throws FlowConfigLoggedException {


surprised to see this private when the interface had this public

no, it did not exist in older FlowConfigsResourceHandler interface. it could be private so changed it to private

sorry, I missed that this overloads the method that IS in the interface, but just w/o the long modifiedWatermark

phet

great work here arjun!

phet · 2024-09-10T21:08:15Z

...er/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java

    // We have modifiedWatermark here to avoid update config happens at the same time on different hosts overwrite each other
    // timestamp here will be treated as largest modifiedWatermark that we can update
    long version = System.currentTimeMillis() / 1000;
    return updateFlowConfig(flowId, flowConfig, version);
  }

-  public UpdateResponse updateFlowConfig(FlowId flowId,
-      FlowConfig flowConfig, long modifiedWatermark) throws FlowConfigLoggedException {
+  private UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig, long modifiedWatermark) throws FlowConfigLoggedException {


sorry, I missed that this overloads the method that IS in the interface, but just w/o the long modifiedWatermark

arjun4084346 force-pushed the goodbyedagmanager branch 2 times, most recently from 7c8d1cc to 45e2ff4 Compare August 29, 2024 22:55

arjun4084346 force-pushed the goodbyedagmanager branch 3 times, most recently from 6d04dbd to da9730a Compare September 8, 2024 01:26

remove flowconfigs resource handler for single active code paths

295b529

arjun4084346 force-pushed the goodbyedagmanager branch from 166b532 to 295b529 Compare September 8, 2024 01:54

arjun4084346 changed the title ~~Goodbyedagmanager~~ [GOBBLIN-2136] consolidate various resource handlers for flowconfigs Sep 9, 2024

arjun4084346 force-pushed the goodbyedagmanager branch from 6d60f81 to 295b529 Compare September 9, 2024 19:35

phet reviewed Sep 9, 2024

View reviewed changes

arjun4084346 added 2 commits September 9, 2024 15:52

address review comments

cf2d4e8

address review comments

6ef073a

phet reviewed Sep 10, 2024

View reviewed changes

address review comments

6be4fe1

phet approved these changes Sep 10, 2024

View reviewed changes

arjun4084346 merged commit 833fed3 into apache:master Sep 10, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-2136] consolidate various resource handlers for flowconfigs #4041

[GOBBLIN-2136] consolidate various resource handlers for flowconfigs #4041

arjun4084346 commented Aug 29, 2024 •

edited

Loading

codecov-commenter commented Sep 8, 2024 •

edited

Loading

phet Sep 9, 2024 •

edited

Loading

arjun4084346 Sep 9, 2024

phet Sep 9, 2024

arjun4084346 Sep 9, 2024

phet Sep 9, 2024

arjun4084346 Sep 9, 2024 •

edited

Loading

phet Sep 10, 2024

arjun4084346 Sep 10, 2024

phet Sep 10, 2024

phet left a comment

phet Sep 10, 2024



		@Slf4j
		public class FlowExecutionResourceHandler implements FlowExecutionResourceHandlerInterface {

[GOBBLIN-2136] consolidate various resource handlers for flowconfigs #4041

[GOBBLIN-2136] consolidate various resource handlers for flowconfigs #4041

Conversation

arjun4084346 commented Aug 29, 2024 • edited Loading

JIRA

Description

Tests

Commits

codecov-commenter commented Sep 8, 2024 • edited Loading

Codecov Report

phet Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjun4084346 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjun4084346 commented Aug 29, 2024 •

edited

Loading

codecov-commenter commented Sep 8, 2024 •

edited

Loading

phet Sep 9, 2024 •

edited

Loading

arjun4084346 Sep 9, 2024 •

edited

Loading