You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/10/06 17:28:10 UTC
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3113: [GOBBLIN-1273] Improving handling of config file load failures
sv2000 commented on a change in pull request #3113:
URL: https://github.com/apache/incubator-gobblin/pull/3113#discussion_r500463929
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/PullFileLoader.java
##########
@@ -284,43 +287,62 @@ private Config findAndLoadGlobalConfigInDirectory(Path path, Config fallback) th
* @return The {@link Config} in path with fallback as fallback.
* @throws IOException
*/
- private Config loadJavaPropsWithFallback(Path propertiesPath, Config fallback) throws IOException {
+ private Config loadJavaPropsWithFallback(Path propertiesPath, Config fallback)
+ throws IOException {
PropertiesConfiguration propertiesConfiguration = new PropertiesConfiguration();
- try (InputStreamReader inputStreamReader = new InputStreamReader(this.fs.open(propertiesPath),
- Charsets.UTF_8)) {
- propertiesConfiguration.setDelimiterParsingDisabled(ConfigUtils.getBoolean(fallback,
- PROPERTY_DELIMITER_PARSING_ENABLED_KEY, DEFAULT_PROPERTY_DELIMITER_PARSING_ENABLED_KEY));
+ try (InputStreamReader inputStreamReader = new InputStreamReader(this.fs.open(propertiesPath), Charsets.UTF_8)) {
+ propertiesConfiguration.setDelimiterParsingDisabled(ConfigUtils
+ .getBoolean(fallback, PROPERTY_DELIMITER_PARSING_ENABLED_KEY, DEFAULT_PROPERTY_DELIMITER_PARSING_ENABLED_KEY));
propertiesConfiguration.load(inputStreamReader);
Config configFromProps =
ConfigUtils.propertiesToConfig(ConfigurationConverter.getProperties(propertiesConfiguration));
return ConfigFactory.parseMap(ImmutableMap.of(ConfigurationKeys.JOB_CONFIG_FILE_PATH_KEY,
- PathUtils.getPathWithoutSchemeAndAuthority(propertiesPath).toString()))
- .withFallback(configFromProps)
+ PathUtils.getPathWithoutSchemeAndAuthority(propertiesPath).toString())).withFallback(configFromProps)
.withFallback(fallback);
} catch (ConfigurationException ce) {
+ log.error("Failed to load Java properties from file at {} due to {}", propertiesPath, ce.getLocalizedMessage());
throw new IOException(ce);
}
}
- private Config loadHoconConfigAtPath(Path path) throws IOException {
- try (InputStream is = fs.open(path);
- Reader reader = new InputStreamReader(is, Charsets.UTF_8)) {
- return ConfigFactory.parseMap(ImmutableMap.of(ConfigurationKeys.JOB_CONFIG_FILE_PATH_KEY,
- PathUtils.getPathWithoutSchemeAndAuthority(path).toString()))
- .withFallback(ConfigFactory.parseReader(reader, ConfigParseOptions.defaults().setSyntax(ConfigSyntax.CONF)));
+ private Config loadHoconConfigAtPath(Path path)
+ throws IOException {
+ try (InputStream is = fs.open(path); Reader reader = new InputStreamReader(is, Charsets.UTF_8)) {
+ return ConfigFactory.parseMap(ImmutableMap
+ .of(ConfigurationKeys.JOB_CONFIG_FILE_PATH_KEY, PathUtils.getPathWithoutSchemeAndAuthority(path).toString()))
+ .withFallback(ConfigFactory.parseReader(reader, ConfigParseOptions.defaults().setSyntax(ConfigSyntax.CONF)));
+ } catch (ConfigException configException) {
+ throw wrapConfigException(path, configException);
}
}
- private Config loadHoconConfigWithFallback(Path path, Config fallback) throws IOException {
- try (InputStream is = fs.open(path);
- Reader reader = new InputStreamReader(is, Charsets.UTF_8)) {
- return ConfigFactory.parseMap(ImmutableMap.of(ConfigurationKeys.JOB_CONFIG_FILE_PATH_KEY,
- PathUtils.getPathWithoutSchemeAndAuthority(path).toString()))
- .withFallback(ConfigFactory.parseReader(reader, ConfigParseOptions.defaults().setSyntax(ConfigSyntax.CONF)))
- .withFallback(fallback);
+ /**
+ * Wrap a {@link ConfigException} (which extends {@link RuntimeException} with an IOException,
+ * with a helpful message if possible
+ * @param path
+ * @param configException
+ * @return an {@link IOException} wrapping the passed in ConfigException.
+ */
+ private IOException wrapConfigException(Path path, ConfigException configException) {
+ if (configException.origin() != null) {
+ return new IOException("Failed to parse config file " + path.toString()
+ + " at lineNo:" + configException.origin().lineNumber(),configException);
Review comment:
Nit: Add a whitespace after the ",".
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/PullFileLoader.java
##########
@@ -175,14 +176,14 @@ public Config apply(@Nullable Path jobFile) {
}
try {
- return PullFileLoader.this.loadPullFile(jobFile,
- sysProps, loadGlobalProperties);
- } catch (IOException e) {
- log.error("Cannot load job from {} due to {}", jobFile, ExceptionUtils.getFullStackTrace(e));
+ return PullFileLoader.this.loadPullFile(jobFile, sysProps, loadGlobalProperties);
+ } catch (IOException ie) {
+ log.error("",ie);
Review comment:
Add white space after ",".
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/PullFileLoader.java
##########
@@ -175,14 +176,14 @@ public Config apply(@Nullable Path jobFile) {
}
try {
- return PullFileLoader.this.loadPullFile(jobFile,
- sysProps, loadGlobalProperties);
- } catch (IOException e) {
- log.error("Cannot load job from {} due to {}", jobFile, ExceptionUtils.getFullStackTrace(e));
+ return PullFileLoader.this.loadPullFile(jobFile, sysProps, loadGlobalProperties);
+ } catch (IOException ie) {
+ log.error("",ie);
return null;
}
}
- });
+ }).stream().filter(x -> x!= null).collect(Collectors.toList());
Review comment:
Add whitespaces around "!=".
##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/PullFileLoader.java
##########
@@ -175,14 +176,14 @@ public Config apply(@Nullable Path jobFile) {
}
try {
- return PullFileLoader.this.loadPullFile(jobFile,
- sysProps, loadGlobalProperties);
- } catch (IOException e) {
- log.error("Cannot load job from {} due to {}", jobFile, ExceptionUtils.getFullStackTrace(e));
+ return PullFileLoader.this.loadPullFile(jobFile, sysProps, loadGlobalProperties);
+ } catch (IOException ie) {
+ log.error("",ie);
return null;
}
}
- });
+ }).stream().filter(x -> x!= null).collect(Collectors.toList());
+ // only return valid parsed configs
Review comment:
Can this behavior be documented in the javadoc of the method?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org