thisisArjit commented on code in PR #4141: URL: https://github.com/apache/gobblin/pull/4141#discussion_r2360524841
########## gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java: ########## Review Comment: nit: add `@Override` to setActive method ########## gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagActionChangeMonitor.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.monitoring; + +import com.google.common.util.concurrent.Service; + + +/** + * A marker interface for dag action change monitors to generalize initialization in {@link org.apache.gobblin.service.modules.core.GobblinServiceManager#dagManagementDagActionStoreChangeMonitor} + */ +public interface DagActionChangeMonitor extends Service { Review Comment: given we are extending Service class, we can explore startAsync / awaitRunning method from that. setActive might not be required then? ########## gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagActionChangeMonitor.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.monitoring; + +import com.google.common.util.concurrent.Service; + + +/** + * A marker interface for dag action change monitors to generalize initialization in {@link org.apache.gobblin.service.modules.core.GobblinServiceManager#dagManagementDagActionStoreChangeMonitor} + */ +public interface DagActionChangeMonitor extends Service { + void setActive(); Review Comment: add java doc for this method ########## gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagManagementDagActionStoreChangeMonitorFactory.java: ########## @@ -30,13 +30,16 @@ import org.apache.gobblin.service.modules.orchestration.DagManagementStateStore; import org.apache.gobblin.service.modules.orchestration.task.DagProcessingEngineMetrics; import org.apache.gobblin.util.ConfigUtils; +import org.apache.gobblin.util.reflection.GobblinConstructorUtils; /** - * A factory implementation that returns a {@link DagManagementDagActionStoreChangeMonitor} instance. + * A factory implementation that returns a {@link DagActionChangeMonitor} instance. */ @Slf4j -public class DagManagementDagActionStoreChangeMonitorFactory implements Provider<DagManagementDagActionStoreChangeMonitor> { +public class DagManagementDagActionStoreChangeMonitorFactory implements Provider<DagActionChangeMonitor> { + static final String DAG_ACTION_STORE_CHANGE_MONITOR_CLASS_KEY = "class"; + static final String DEFAULT_DAG_ACTION_STORE_CHANGE_MONITOR_CLASS = DagManagementDagActionStoreChangeMonitor.class.getName(); Review Comment: private static final? ########## gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/SpecStoreChangeMonitor.java: ########## @@ -51,7 +51,7 @@ * a connector between the API and execution layers of GaaS. */ @Slf4j -public class SpecStoreChangeMonitor extends HighLevelConsumer<String, GenericStoreChangeEvent> { +public class SpecStoreChangeMonitor extends HighLevelConsumer<String, GenericStoreChangeEvent> implements SpecChangeMonitor{ Review Comment: nit: missing space after SpecChangeMonitor ########## gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/SpecChangeMonitor.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.monitoring; + +import com.google.common.util.concurrent.Service; + + +/** + * A marker interface for spec change monitors to generalize initialization in {@link org.apache.gobblin.service.modules.core.GobblinServiceManager#specStoreChangeMonitor} + */ +public interface SpecChangeMonitor extends Service { + void setActive(); Review Comment: see DagActionChangeMonitor comments ########## gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/SpecStoreChangeMonitorFactory.java: ########## @@ -28,13 +28,16 @@ import org.apache.gobblin.runtime.spec_catalog.FlowCatalog; import org.apache.gobblin.service.modules.scheduler.GobblinServiceJobScheduler; import org.apache.gobblin.util.ConfigUtils; +import org.apache.gobblin.util.reflection.GobblinConstructorUtils; /** - * A factory implementation that returns a {@link SpecStoreChangeMonitor} instance. + * A factory implementation that returns a {@link SpecChangeMonitor} instance. */ @Slf4j -public class SpecStoreChangeMonitorFactory implements Provider<SpecStoreChangeMonitor> { +public class SpecStoreChangeMonitorFactory implements Provider<SpecChangeMonitor> { + static final String SPEC_STORE_CHANGE_MONITOR_CLASS_KEY = "class"; + static final String DEFAULT_SPEC_STORE_CHANGE_MONITOR_CLASS = SpecStoreChangeMonitor.class.getName(); Review Comment: private static? -- 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]
