-
Notifications
You must be signed in to change notification settings - Fork 750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GOBBLIN-2136] consolidate various resource handlers for flowconfigs #4041
Conversation
7c8d1cc
to
45e2ff4
Compare
6d04dbd
to
da9730a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
166b532
to
295b529
Compare
6d60f81
to
295b529
Compare
...n-flow-config-service-api/src/main/idl/org.apache.gobblin.service.flowstatuses.restspec.json
Outdated
Show resolved
Hide resolved
...w-config-service-api/src/main/snapshot/org.apache.gobblin.service.flowstatuses.snapshot.json
Outdated
Show resolved
Hide resolved
...lin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java
Outdated
Show resolved
Hide resolved
protected void assignTopicPartitions() { | ||
// Expects underlying consumer to handle initializing partitions and offset for the topic - | ||
// subscribe to all partitions from latest offset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
|
||
|
||
@Slf4j | ||
public class FlowExecutionResourceHandler implements FlowExecutionResourceHandlerInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javadoc?
also, the diff isn't showing this as a rename, but is that what it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added javadoc. github issues
...vice-server/src/test/java/org/apache/gobblin/service/FlowConfigResourceLocalHandlerTest.java
Outdated
Show resolved
Hide resolved
...e-server/src/test/java/org/apache/gobblin/service/FlowExecutionResourceLocalHandlerTest.java
Outdated
Show resolved
Hide resolved
@Slf4j | ||
public class FlowConfigResourceLocalHandler implements FlowConfigsResourceHandler { | ||
public class FlowConfigsV2ResourceHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not implement a FlowConfigsV2ResourceHandlerInterface
, like flowExecutions does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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
Outdated
Show resolved
Hide resolved
...ice-server/src/main/java/org/apache/gobblin/service/FlowConfigsResourceHandlerInterface.java
Outdated
Show resolved
Hide resolved
...e-server/src/main/java/org/apache/gobblin/service/FlowExecutionResourceHandlerInterface.java
Outdated
Show resolved
Hide resolved
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surprised to see this private when the interface had this public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it did not exist in older FlowConfigsResourceHandler
interface. it could be private so changed it to private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I missed that this overloads the method that IS in the interface, but just w/o the long modifiedWatermark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work here arjun!
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I missed that this overloads the method that IS in the interface, but just w/o the long modifiedWatermark
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
Description
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
no functiotnality changes
Commits