phet commented on code in PR #3899:
URL: https://github.com/apache/gobblin/pull/3899#discussion_r1540572517
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java:
##########
@@ -265,6 +264,9 @@ public static GobblinServiceManager
create(GobblinServiceConfiguration serviceCo
* @param <T>
*/
public static <T> T getClass(Class<T> classToGet) {
+ if (GOBBLIN_SERVICE_GUICE_MODULE == null) {
+ throw new RuntimeException("getClass called without calling create
method to initialize GobblinServiceGuiceModule");
Review Comment:
include `classToGet` in the message so another clue toward who called it
prematurely (in addition to stack trace)
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MultiActiveLeaseArbiter.java:
##########
@@ -117,12 +117,16 @@ public long getEventTimeMillis() {
}
/**
- * Completes the lease referenced by this status object if it has not
expired.
+ * Completes the lease referenced by this status object if it has not
expired. Defaults to true if no lease arbiter
+ * to complete lease on.
* @return true if able to complete lease, false otherwise.
* @throws IOException
*/
public boolean completeLease() throws IOException {
- return multiActiveLeaseArbiter.recordLeaseSuccess(this);
+ if (multiActiveLeaseArbiter != null) {
Review Comment:
`Optional` (not `null`)
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MultiActiveLeaseArbiterFactory.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.runtime.api;
+
+import java.io.IOException;
+import java.util.Objects;
+
+import com.typesafe.config.Config;
+
+import javax.inject.Provider;
+import lombok.extern.slf4j.Slf4j;
+
+
+/**
+ * An abstract base class for creating {@link MultiActiveLeaseArbiter}
factories that use a specific configuration key.
Review Comment:
nit: drop "creating"
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MultiActiveLeaseArbiterFactory.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.runtime.api;
+
+import java.io.IOException;
+import java.util.Objects;
+
+import com.typesafe.config.Config;
+
+import javax.inject.Provider;
+import lombok.extern.slf4j.Slf4j;
+
+
+/**
+ * An abstract base class for creating {@link MultiActiveLeaseArbiter}
factories that use a specific configuration key.
+ * Subclasses must provide a key to use in the constructor.
+ */
+@Slf4j
+public abstract class MultiActiveLeaseArbiterFactory implements
Provider<MultiActiveLeaseArbiter> {
+ private final Config config;
+ private final String key;
+
+ public MultiActiveLeaseArbiterFactory(Config config, String key) {
+ this.config = Objects.requireNonNull(config);
+ this.key = Objects.requireNonNull(key);
+ }
+
+ @Override
+ public MultiActiveLeaseArbiter get() {
+ try {
+ Config leaseArbiterConfig = this.config.getConfig(key);
Review Comment:
wouldn't this throw if `key` does not exist in `config`? if so, suggest to
verify in ctor and fail to even construct an instance when an error in params
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/DagProc.java:
##########
@@ -36,6 +37,7 @@
* actions based on the type of {@link DagTask} and finally submitting an
event to the executor.
*/
@Alpha
+@RequiredArgsConstructor
Review Comment:
remove `@ReqArgsCtor`, since `@Data`
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/DagProc.java:
##########
@@ -61,5 +64,7 @@ public final void process(DagManagementStateStore
dagManagementStateStore) throw
protected abstract void sendNotification(T result, EventSubmitter
eventSubmitter) throws IOException;
// todo - commit the modified dags to the persistent store, maybe not
required for InMem dagManagementStateStore
- protected abstract void commit(DagManagementStateStore
dagManagementStateStore, T result);
+ protected void commit(DagManagementStateStore dagManagementStateStore, T
result) {
+ dagTask.conclude();
+ }
Review Comment:
sorry if I led you astray, but this impl here seems unnecessary, as
`DagProcEngine` [deals
directly](https://github.com/apache/gobblin/blob/de50eaf54e496a01e8a0759d942572d1359b5f98/gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngine.java#L98)
w/ the `DagTask`.
let's remove the method entirely
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
