You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2020/10/06 17:29:00 UTC
[jira] [Work logged] (GOBBLIN-1273) Pull or Conf file loading
failures are not easy to understand
[ https://issues.apache.org/jira/browse/GOBBLIN-1273?focusedWorklogId=496048&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-496048 ]
ASF GitHub Bot logged work on GOBBLIN-1273:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 06/Oct/20 17:28
Start Date: 06/Oct/20 17:28
Worklog Time Spent: 10m
Work Description: 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
Issue Time Tracking
-------------------
Worklog Id: (was: 496048)
Remaining Estimate: 0h
Time Spent: 10m
> Pull or Conf file loading failures are not easy to understand
> -------------------------------------------------------------
>
> Key: GOBBLIN-1273
> URL: https://issues.apache.org/jira/browse/GOBBLIN-1273
> Project: Apache Gobblin
> Issue Type: Improvement
> Reporter: Shirshanka Das
> Priority: Minor
> Time Spent: 10m
> Remaining Estimate: 0h
>
> When config files fail to load (specifically hocon files), Gobblin doesn't log which file and which line number caused the failure.
>
> e.g. if a .conf file with unquoted : characters is found in a standalone server's config directory, the startup aborts with the following message:
>
> _Exception in thread "main" java.lang.IllegalStateException: Expected to be healthy after starting_
> _at com.google.common.base.Preconditions.checkState(Preconditions.java:150)_
> _at com.google.common.util.concurrent.ServiceManager.awaitHealthy(ServiceManager.java:278)_
> _at org.apache.gobblin.runtime.app.ServiceBasedAppLauncher.start(ServiceBasedAppLauncher.java:168)_
> _at org.apache.gobblin.scheduler.SchedulerDaemon.main(SchedulerDaemon.java:75)_
> _Exception in thread "JobScheduler STARTING" com.typesafe.config.ConfigException$BadPath: Reader: 30: Token not allowed in path expression: ':' (you can double-quote this token if you really want it here)_
> _at com.typesafe.config.impl.Parser.parsePathExpression(Parser.java:1095)_
> _at com.typesafe.config.impl.Parser.parsePathExpression(Parser.java:1049)_
> _at com.typesafe.config.impl.Parser.access$000(Parser.java:27)_
> _..._
>
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)