phet commented on code in PR #3536:
URL: https://github.com/apache/gobblin/pull/3536#discussion_r947326733
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/GitConfigListener.java:
##########
@@ -14,83 +14,64 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.gobblin.service.modules.core;
-import java.io.IOException;
-
-import org.apache.hadoop.fs.Path;
-import org.eclipse.jgit.diff.DiffEntry;
+package org.apache.gobblin.service.monitoring;
-import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Sets;
import com.google.common.io.Files;
import com.typesafe.config.Config;
import com.typesafe.config.ConfigFactory;
import com.typesafe.config.ConfigValueFactory;
-
-import javax.inject.Inject;
-import javax.inject.Singleton;
+import java.io.IOException;
+import java.net.URI;
+import java.util.Set;
import lombok.extern.slf4j.Slf4j;
-
import org.apache.gobblin.config.ConfigBuilder;
import org.apache.gobblin.configuration.ConfigurationKeys;
import org.apache.gobblin.runtime.api.FlowSpec;
import org.apache.gobblin.runtime.spec_catalog.FlowCatalog;
import org.apache.gobblin.runtime.spec_store.FSSpecStore;
import org.apache.gobblin.util.PullFileLoader;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.eclipse.jgit.diff.DiffEntry;
+
/**
- * Service that monitors for jobs from a git repository.
- * The git repository must have an initial commit that has no config files
since that is used as a base for getting
- * the change list.
- * The config needs to be organized with the following structure:
- * <root_config_dir>/<flowGroup>/<flowName>.(pull|job|json|conf)
- * The <flowGroup> and <flowName> is used to generate the URI used to store
the config in the {@link FlowCatalog}
+ * Listener for {@link GitConfigMonitor} to apply changes in Git to a {@link
FlowCatalog} for adding and removing jobs
Review Comment:
probably *changes from Git* ... so it doesn't sound like the changes are
applied in/to git.
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/GitConfigListener.java:
##########
@@ -137,23 +118,23 @@ public void removeChange(DiffEntry change) {
.withDescription(SPEC_DESCRIPTION)
.build();
- this.flowCatalog.remove(spec.getUri());
+ this.flowCatalog.remove(spec.getUri());
}
}
- /**
- * check whether the file has the proper naming and hierarchy
- * @param configFilePath the relative path from the repo root
- * @return false if the file does not conform
- */
+ /**
+ * check whether the file has the proper naming and hierarchy
+ * @param configFilePath the relative path from the repo root
+ * @return false if the file does not conform
+ */
private boolean checkConfigFilePath(String configFilePath) {
// The config needs to stored at
configDir/flowGroup/flowName.(pull|job|json|conf)
Path configFile = new Path(configFilePath);
String fileExtension = Files.getFileExtension(configFile.getName());
if (configFile.depth() != CONFIG_FILE_DEPTH
- || !configFile.getParent().getParent().getName().equals(folderName)
+ ||
!configFile.getParent().getParent().getName().equals(configBaseFolderName)
||
!(PullFileLoader.DEFAULT_JAVA_PROPS_PULL_FILE_EXTENSIONS.contains(fileExtension)
||
PullFileLoader.DEFAULT_JAVA_PROPS_PULL_FILE_EXTENSIONS.contains(fileExtension)))
{
log.warn("Changed file does not conform to directory structure and file
name format, skipping: "
Review Comment:
I get not failing the entire service--but seems a particularly soft error to
merely log a warning... how often do people notice this, I wonder...
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/GitConfigMonitor.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.collect.ImmutableMap;
+import com.typesafe.config.Config;
+import com.typesafe.config.ConfigFactory;
+
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.gobblin.configuration.ConfigurationKeys;
+import org.apache.gobblin.runtime.spec_catalog.FlowCatalog;
+
+/**
+ * Service that monitors for jobs from a git repository.
Review Comment:
are these jobs, as described, or flows?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java:
##########
@@ -32,6 +32,7 @@
import org.apache.commons.cli.ParseException;
import org.apache.commons.lang3.ObjectUtils;
import org.apache.gobblin.service.modules.orchestration.UserQuotaManager;
+import org.apache.gobblin.service.monitoring.GitConfigMonitor;
Review Comment:
I see only this one change in the file. why is it necessary to import if
it's not used anywhere?
--
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]
