You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "arjun4084346 (via GitHub)" <gi...@apache.org> on 2024/08/29 22:27:05 UTC
[PR] Goodbyedagmanager [gobblin]
arjun4084346 opened a new pull request, #4041:
URL: https://github.com/apache/gobblin/pull/4041
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](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-XXX
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### 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](http://chris.beams.io/posts/git-commit/)":
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"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750995487
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -60,14 +63,22 @@
import org.apache.gobblin.runtime.api.SpecNotFoundException;
import org.apache.gobblin.runtime.spec_catalog.AddSpecResponse;
import org.apache.gobblin.runtime.spec_catalog.FlowCatalog;
+import org.apache.gobblin.runtime.util.InjectionNames;
+import org.apache.gobblin.service.FlowConfig;
+import org.apache.gobblin.service.FlowConfigLoggedException;
+import org.apache.gobblin.service.FlowId;
+import org.apache.gobblin.service.FlowStatusId;
+import org.apache.gobblin.service.RequesterService;
+import org.apache.gobblin.service.Schedule;
+import org.apache.gobblin.service.ServiceConfigKeys;
import org.apache.gobblin.util.ConfigUtils;
-/**
- * A {@link FlowConfigsResourceHandler} that handles Restli locally.
- */
@Slf4j
-public class FlowConfigResourceLocalHandler implements FlowConfigsResourceHandler {
+public class FlowConfigsV2ResourceHandler {
Review Comment:
```
// 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-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -60,14 +63,22 @@
import org.apache.gobblin.runtime.api.SpecNotFoundException;
import org.apache.gobblin.runtime.spec_catalog.AddSpecResponse;
import org.apache.gobblin.runtime.spec_catalog.FlowCatalog;
+import org.apache.gobblin.runtime.util.InjectionNames;
+import org.apache.gobblin.service.FlowConfig;
+import org.apache.gobblin.service.FlowConfigLoggedException;
+import org.apache.gobblin.service.FlowId;
+import org.apache.gobblin.service.FlowStatusId;
+import org.apache.gobblin.service.RequesterService;
+import org.apache.gobblin.service.Schedule;
+import org.apache.gobblin.service.ServiceConfigKeys;
import org.apache.gobblin.util.ConfigUtils;
-/**
- * A {@link FlowConfigsResourceHandler} that handles Restli locally.
- */
@Slf4j
-public class FlowConfigResourceLocalHandler implements FlowConfigsResourceHandler {
+public class FlowConfigsV2ResourceHandler {
Review Comment:
```
// 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1751150699
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowExecutionResourceHandlerInterface.java:
##########
@@ -25,8 +25,8 @@
import com.linkedin.restli.server.UpdateResponse;
-// Unlike FlowConfigsV2ResourceHandler, this 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,
+// This is an interface rather than a class because implementation may need resources from the packages it cannot have
+// direct dependency on
public interface FlowExecutionResourceHandlerInterface {
Review Comment:
also javadoc
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -164,16 +165,14 @@ public UpdateResponse partialUpdateFlowConfig(FlowId flowId,
return updateFlowConfig(flowId, flowConfig, modifiedWatermark);
}
- public UpdateResponse updateFlowConfig(FlowId flowId,
- FlowConfig flowConfig) throws FlowConfigLoggedException {
+ public UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig) throws FlowConfigLoggedException {
// 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 {
Review Comment:
surprised to see this private when the interface had this public
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigsResourceHandlerInterface.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.service;
+
+import java.util.Collection;
+import java.util.Properties;
+
+import com.linkedin.restli.common.ComplexResourceKey;
+import com.linkedin.restli.common.PatchRequest;
+import com.linkedin.restli.server.CreateKVResponse;
+import com.linkedin.restli.server.UpdateResponse;
+
+import org.apache.gobblin.runtime.api.FlowSpecSearchObject;
+
+
+// This is an interface rather than a class because implementation may need resources from the packages it cannot have
+// direct dependency on
+public interface FlowConfigsResourceHandlerInterface {
Review Comment:
good comment, but deserves more javadoc, esp. citing the PDL endpoint this relates to. if it's not clear from that, potentially do call out that this relates to the flowConfigsV2 endpoint (not flowConfigs, which has been removed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750769923
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -15,7 +15,7 @@
* limitations under the License.
*/
-package org.apache.gobblin.service;
+package org.apache.gobblin.service.modules.restli;
Review Comment:
moving it back to the original package. not committing to not disrupt the review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750642724
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-api/src/main/snapshot/org.apache.gobblin.service.flowstatuses.snapshot.json:
##########
@@ -264,11 +264,14 @@
"deprecated" : "Use FlowExecution instead"
}, "org.apache.gobblin.service.FlowStatusId", "org.apache.gobblin.service.Issue", "org.apache.gobblin.service.IssueSeverity", "org.apache.gobblin.service.JobId", "org.apache.gobblin.service.JobState", "org.apache.gobblin.service.JobStatistics", "org.apache.gobblin.service.JobStatus", "org.apache.gobblin.service.Timestamp" ],
"schema" : {
+ "annotations" : {
+ "deprecated" : { }
+ },
"name" : "flowstatuses",
"namespace" : "org.apache.gobblin.service",
"path" : "/flowstatuses",
"schema" : "org.apache.gobblin.service.FlowStatus",
- "doc" : "Resource for handling flow status requests\n\ngenerated from: org.apache.gobblin.service.FlowStatusResource",
+ "doc" : "Resource for handling flow status requests. Deprecated, use {@link FlowExecutionResource}\n\ngenerated from: org.apache.gobblin.service.FlowStatusResource",
Review Comment:
same advice here
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/restli/FlowExecutionResourceHandler.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.service.modules.restli;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.commons.lang.StringUtils;
+
+import com.google.inject.Inject;
+import com.linkedin.restli.common.ComplexResourceKey;
+import com.linkedin.restli.common.EmptyRecord;
+import com.linkedin.restli.common.HttpStatus;
+import com.linkedin.restli.server.PagingContext;
+import com.linkedin.restli.server.RestLiServiceException;
+import com.linkedin.restli.server.UpdateResponse;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.service.FlowExecution;
+import org.apache.gobblin.service.FlowExecutionResource;
+import org.apache.gobblin.service.FlowExecutionResourceHandlerInterface;
+import org.apache.gobblin.service.FlowId;
+import org.apache.gobblin.service.FlowStatusId;
+import org.apache.gobblin.service.modules.orchestration.DagActionStore;
+import org.apache.gobblin.service.modules.orchestration.DagManagementStateStore;
+import org.apache.gobblin.service.monitoring.FlowStatus;
+import org.apache.gobblin.service.monitoring.FlowStatusGenerator;
+
+
+@Slf4j
+public class FlowExecutionResourceHandler implements FlowExecutionResourceHandlerInterface {
Review Comment:
javadoc?
also, the diff isn't showing this as a rename, but is that what it is?
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-api/src/main/idl/org.apache.gobblin.service.flowstatuses.restspec.json:
##########
@@ -1,9 +1,12 @@
{
+ "annotations" : {
+ "deprecated" : { }
+ },
"name" : "flowstatuses",
"namespace" : "org.apache.gobblin.service",
"path" : "/flowstatuses",
"schema" : "org.apache.gobblin.service.FlowStatus",
- "doc" : "Resource for handling flow status requests\n\ngenerated from: org.apache.gobblin.service.FlowStatusResource",
+ "doc" : "Resource for handling flow status requests. Deprecated, use {@link FlowExecutionResource}\n\ngenerated from: org.apache.gobblin.service.FlowStatusResource",
Review Comment:
suggest to announce deprecation first:
```
Deprecated, use {@link FlowExecutionResource}\n\nResource for handling flow status requests\n\ngenerated from: org.apache.gobblin.service.FlowStatusResource",
```
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java:
##########
@@ -289,7 +280,7 @@ private void registerServicesInLauncher(){
private void configureServices(){
if (configuration.isRestLIServerEnabled()) {
this.restliServer = EmbeddedRestliServer.builder()
- .resources(Lists.newArrayList(FlowConfigsResource.class, FlowConfigsV2Resource.class))
+ .resources(Lists.newArrayList(FlowConfigsV2Resource.class, FlowConfigsV2Resource.class))
Review Comment:
can we list just once?
##########
gobblin-service/src/test/java/org/apache/gobblin/service/FlowExecutionTest.java:
##########
@@ -30,29 +30,28 @@
import com.google.common.base.Suppliers;
import com.google.common.collect.Lists;
-import com.google.inject.Binder;
import com.google.inject.Guice;
import com.google.inject.Injector;
-import com.google.inject.Module;
import org.apache.gobblin.configuration.State;
import org.apache.gobblin.metastore.StateStore;
import org.apache.gobblin.restli.EmbeddedRestliServer;
import org.apache.gobblin.runtime.troubleshooter.MultiContextIssueRepository;
+import org.apache.gobblin.service.modules.orchestration.DagManagementStateStore;
+import org.apache.gobblin.service.modules.restli.FlowExecutionResourceHandler;
import org.apache.gobblin.service.monitoring.FlowStatusGenerator;
import org.apache.gobblin.service.monitoring.JobStatusRetriever;
import static org.mockito.Mockito.mock;
@Test(groups = { "gobblin.service" }, singleThreaded = true)
-public class FlowStatusTest {
- private FlowStatusClient _client;
+public class FlowExecutionTest {
Review Comment:
`FlowExecutionClientSideTest`?
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -60,14 +63,22 @@
import org.apache.gobblin.runtime.api.SpecNotFoundException;
import org.apache.gobblin.runtime.spec_catalog.AddSpecResponse;
import org.apache.gobblin.runtime.spec_catalog.FlowCatalog;
+import org.apache.gobblin.runtime.util.InjectionNames;
+import org.apache.gobblin.service.FlowConfig;
+import org.apache.gobblin.service.FlowConfigLoggedException;
+import org.apache.gobblin.service.FlowId;
+import org.apache.gobblin.service.FlowStatusId;
+import org.apache.gobblin.service.RequesterService;
+import org.apache.gobblin.service.Schedule;
+import org.apache.gobblin.service.ServiceConfigKeys;
import org.apache.gobblin.util.ConfigUtils;
-/**
- * A {@link FlowConfigsResourceHandler} that handles Restli locally.
- */
@Slf4j
-public class FlowConfigResourceLocalHandler implements FlowConfigsResourceHandler {
+public class FlowConfigsV2ResourceHandler {
Review Comment:
why not implement a `FlowConfigsV2ResourceHandlerInterface`, like flowExecutions does?
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/test/java/org/apache/gobblin/service/FlowConfigResourceLocalHandlerTest.java:
##########
@@ -29,6 +29,7 @@
import org.apache.gobblin.configuration.ConfigurationKeys;
import org.apache.gobblin.runtime.api.FlowSpec;
+import org.apache.gobblin.service.modules.restli.FlowConfigsV2ResourceHandler;
public class FlowConfigResourceLocalHandlerTest {
Review Comment:
shouldn't this be renamed to V2? also, hasn't the "resource local handler" naming convention also gone away?
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagFlowTest.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.service.modules.orchestration;
+
+import java.util.concurrent.TimeUnit;
+
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigValueFactory;
+
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.service.ServiceConfigKeys;
+import org.apache.gobblin.service.modules.flowgraph.Dag;
+import org.apache.gobblin.service.modules.spec.JobExecutionPlan;
+
+
+/**
+ * Tests the state updates (including updating in-memory state and MysqlDagActionStore) after performing add or cancel
+ * operations by calling addDag, stopDag, kill, and resume. It also tests flows with and without sla configs.
+ */
+public class DagFlowTest {
+
+ @Test
+ void slaConfigCheck() throws Exception {
+ Dag<JobExecutionPlan> dag = DagTestUtils.buildDag("5", 123456783L, "FINISH_RUNNING", 1);
+ Assert.assertEquals(DagUtils.getFlowFinishDeadline(dag.getStartNodes().get(0)), ServiceConfigKeys.DEFAULT_FLOW_FINISH_DEADLINE_MILLIS);
+
+ Config jobConfig = dag.getStartNodes().get(0).getValue().getJobSpec().getConfig();
+ jobConfig = jobConfig
+ .withValue(ConfigurationKeys.GOBBLIN_FLOW_FINISH_DEADLINE_TIME, ConfigValueFactory.fromAnyRef("7"))
+ .withValue(ConfigurationKeys.GOBBLIN_FLOW_FINISH_DEADLINE_TIME_UNIT, ConfigValueFactory.fromAnyRef(TimeUnit.SECONDS.name()));
+ dag.getStartNodes().get(0).getValue().getJobSpec().setConfig(jobConfig);
+ Assert.assertEquals(DagUtils.getFlowFinishDeadline(dag.getStartNodes().get(0)), TimeUnit.SECONDS.toMillis(7L));
+
+ jobConfig = jobConfig
Review Comment:
can we use a fresh name, like `jobConfig2`?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java:
##########
@@ -114,7 +114,6 @@ Metrics need to be created before initializeMonitor() below is called (or more s
protected void assignTopicPartitions() {
// Expects underlying consumer to handle initializing partitions and offset for the topic -
// subscribe to all partitions from latest offset
Review Comment:
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 name the specific method involved in the "underlying consumer"?
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagFlowTest.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.service.modules.orchestration;
+
+import java.util.concurrent.TimeUnit;
+
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigValueFactory;
+
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.service.ServiceConfigKeys;
+import org.apache.gobblin.service.modules.flowgraph.Dag;
+import org.apache.gobblin.service.modules.spec.JobExecutionPlan;
+
+
+/**
+ * Tests the state updates (including updating in-memory state and MysqlDagActionStore) after performing add or cancel
+ * operations by calling addDag, stopDag, kill, and resume. It also tests flows with and without sla configs.
+ */
+public class DagFlowTest {
+
+ @Test
+ void slaConfigCheck() throws Exception {
Review Comment:
this javadoc talks about a lot, but it seems mostly like just testing `DagUtils.getFlowFinishDeadline`. is there more than that, which I'm not grasping?
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/test/java/org/apache/gobblin/service/FlowExecutionResourceLocalHandlerTest.java:
##########
@@ -17,50 +17,49 @@
package org.apache.gobblin.service;
-
import org.testng.Assert;
import org.testng.annotations.Test;
public class FlowExecutionResourceLocalHandlerTest {
Review Comment:
let's rename, since the `FlowExecutionResourceLocalHandler` looks to have been deleted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750660625
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java:
##########
@@ -114,7 +114,6 @@ Metrics need to be created before initializeMonitor() below is called (or more s
protected void assignTopicPartitions() {
// Expects underlying consumer to handle initializing partitions and offset for the topic -
// subscribe to all partitions from latest offset
Review Comment:
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"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750995487
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -60,14 +63,22 @@
import org.apache.gobblin.runtime.api.SpecNotFoundException;
import org.apache.gobblin.runtime.spec_catalog.AddSpecResponse;
import org.apache.gobblin.runtime.spec_catalog.FlowCatalog;
+import org.apache.gobblin.runtime.util.InjectionNames;
+import org.apache.gobblin.service.FlowConfig;
+import org.apache.gobblin.service.FlowConfigLoggedException;
+import org.apache.gobblin.service.FlowId;
+import org.apache.gobblin.service.FlowStatusId;
+import org.apache.gobblin.service.RequesterService;
+import org.apache.gobblin.service.Schedule;
+import org.apache.gobblin.service.ServiceConfigKeys;
import org.apache.gobblin.util.ConfigUtils;
-/**
- * A {@link FlowConfigsResourceHandler} that handles Restli locally.
- */
@Slf4j
-public class FlowConfigResourceLocalHandler implements FlowConfigsResourceHandler {
+public class FlowConfigsV2ResourceHandler {
Review Comment:
```// 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750769923
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -15,7 +15,7 @@
* limitations under the License.
*/
-package org.apache.gobblin.service;
+package org.apache.gobblin.service.modules.restli;
Review Comment:
moved it back to the original package. not committing in middle of review to not disrupt it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1751013457
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagFlowTest.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.service.modules.orchestration;
+
+import java.util.concurrent.TimeUnit;
+
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigValueFactory;
+
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.service.ServiceConfigKeys;
+import org.apache.gobblin.service.modules.flowgraph.Dag;
+import org.apache.gobblin.service.modules.spec.JobExecutionPlan;
+
+
+/**
+ * Tests the state updates (including updating in-memory state and MysqlDagActionStore) after performing add or cancel
+ * operations by calling addDag, stopDag, kill, and resume. It also tests flows with and without sla configs.
+ */
+public class DagFlowTest {
+
+ @Test
+ void slaConfigCheck() throws Exception {
Review Comment:
moved to DagUtilsTest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750779068
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/restli/FlowExecutionResourceHandler.java:
##########
@@ -0,0 +1,143 @@
+/*
Review Comment:
moved it to org.apache.gobblin.service package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750681512
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/test/java/org/apache/gobblin/service/FlowConfigResourceLocalHandlerTest.java:
##########
@@ -29,6 +29,7 @@
import org.apache.gobblin.configuration.ConfigurationKeys;
import org.apache.gobblin.runtime.api.FlowSpec;
+import org.apache.gobblin.service.modules.restli.FlowConfigsV2ResourceHandler;
public class FlowConfigResourceLocalHandlerTest {
Review Comment:
shouldn't this be renamed to V2? also, hasn't the "resource local handler" naming convention gone away?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1752670124
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -164,16 +165,14 @@ public UpdateResponse partialUpdateFlowConfig(FlowId flowId,
return updateFlowConfig(flowId, flowConfig, modifiedWatermark);
}
- public UpdateResponse updateFlowConfig(FlowId flowId,
- FlowConfig flowConfig) throws FlowConfigLoggedException {
+ public UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig) throws FlowConfigLoggedException {
// 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 {
Review Comment:
no, it did not exist in older `FlowConfigsResourceHandler` interface. it could be private so changed it to private
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1752722224
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -164,16 +165,14 @@ public UpdateResponse partialUpdateFlowConfig(FlowId flowId,
return updateFlowConfig(flowId, flowConfig, modifiedWatermark);
}
- public UpdateResponse updateFlowConfig(FlowId flowId,
- FlowConfig flowConfig) throws FlowConfigLoggedException {
+ public UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig) throws FlowConfigLoggedException {
// 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 {
Review Comment:
sorry, I missed that this overloads the method that IS in the interface, but just w/o the `long modifiedWatermark`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1751053028
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java:
##########
@@ -114,7 +114,6 @@ Metrics need to be created before initializeMonitor() below is called (or more s
protected void assignTopicPartitions() {
// Expects underlying consumer to handle initializing partitions and offset for the topic -
// subscribe to all partitions from latest offset
Review Comment:
I think all the implementations of HLC except this one needs this method and this is implemented in HLC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 merged PR #4041:
URL: https://github.com/apache/gobblin/pull/4041
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750769923
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -15,7 +15,7 @@
* limitations under the License.
*/
-package org.apache.gobblin.service;
+package org.apache.gobblin.service.modules.restli;
Review Comment:
moved it back to the original package. not committing in middle of review to not disrupt it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1751004646
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/restli/FlowExecutionResourceHandler.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.gobblin.service.modules.restli;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.commons.lang.StringUtils;
+
+import com.google.inject.Inject;
+import com.linkedin.restli.common.ComplexResourceKey;
+import com.linkedin.restli.common.EmptyRecord;
+import com.linkedin.restli.common.HttpStatus;
+import com.linkedin.restli.server.PagingContext;
+import com.linkedin.restli.server.RestLiServiceException;
+import com.linkedin.restli.server.UpdateResponse;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.service.FlowExecution;
+import org.apache.gobblin.service.FlowExecutionResource;
+import org.apache.gobblin.service.FlowExecutionResourceHandlerInterface;
+import org.apache.gobblin.service.FlowId;
+import org.apache.gobblin.service.FlowStatusId;
+import org.apache.gobblin.service.modules.orchestration.DagActionStore;
+import org.apache.gobblin.service.modules.orchestration.DagManagementStateStore;
+import org.apache.gobblin.service.monitoring.FlowStatus;
+import org.apache.gobblin.service.monitoring.FlowStatusGenerator;
+
+
+@Slf4j
+public class FlowExecutionResourceHandler implements FlowExecutionResourceHandlerInterface {
Review Comment:
added javadoc. github issues
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Goodbyedagmanager [gobblin]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#issuecomment-2336509426
## [Codecov](https://app.codecov.io/gh/apache/gobblin/pull/4041?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 41.09%. Comparing base [(`45ad13e`)](https://app.codecov.io/gh/apache/gobblin/commit/45ad13e07ebe212002b279132e1fa4d121a9294f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`166b532`)](https://app.codecov.io/gh/apache/gobblin/commit/166b532609946990b38de5391ef65919b3288734?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #4041 +/- ##
============================================
- Coverage 45.12% 41.09% -4.03%
+ Complexity 3199 2201 -998
============================================
Files 705 480 -225
Lines 26949 20366 -6583
Branches 2680 2355 -325
============================================
- Hits 12160 8370 -3790
+ Misses 13781 11103 -2678
+ Partials 1008 893 -115
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/gobblin/pull/4041?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750769923
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigsV2ResourceHandler.java:
##########
@@ -15,7 +15,7 @@
* limitations under the License.
*/
-package org.apache.gobblin.service;
+package org.apache.gobblin.service.modules.restli;
Review Comment:
moving it back to the original package. not committing in middle of review to not disrupt it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2136] consolidate various resource handlers for flowconfigs [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #4041:
URL: https://github.com/apache/gobblin/pull/4041#discussion_r1750779068
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/restli/FlowExecutionResourceHandler.java:
##########
@@ -0,0 +1,143 @@
+/*
Review Comment:
moved it to org.apache.gobblin.service package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org