You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/12/04 16:21:24 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #1822: Closes #1349 - Throw errors from Main instead of using System.exit()
dlmarion opened a new pull request #1822:
URL: https://github.com/apache/accumulo/pull/1822
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1822: Closes #1349 - Throw errors from Main instead of using System.exit()
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1822:
URL: https://github.com/apache/accumulo/pull/1822#discussion_r536516295
##########
File path: start/src/main/java/org/apache/accumulo/start/Main.java
##########
@@ -41,63 +41,57 @@
private static Class<?> vfsClassLoader;
private static Map<String,KeywordExecutable> servicesMap;
- public static void main(final String[] args) {
+ public static void main(final String[] args) throws Exception {
+ // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when
+ // using the VFSClassLoader with jars in HDFS.
+ ClassLoader loader = getClassLoader();
+ Class<?> confClass = null;
try {
- // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when
- // using the VFSClassLoader with jars in HDFS.
- ClassLoader loader = getClassLoader();
- Class<?> confClass = null;
- try {
- @SuppressWarnings("deprecation")
- var deprecatedConfClass = org.apache.accumulo.start.classloader.AccumuloClassLoader
- .getClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
- confClass = deprecatedConfClass;
- } catch (ClassNotFoundException e) {
- log.error("Unable to find Hadoop Configuration class on classpath, check configuration.",
- e);
- System.exit(1);
- }
- Object conf = null;
- try {
- conf = confClass.getDeclaredConstructor().newInstance();
- } catch (Exception e) {
- log.error("Error creating new instance of Hadoop Configuration", e);
- System.exit(1);
- }
- try {
- Method getClassByNameOrNullMethod =
- conf.getClass().getMethod("getClassByNameOrNull", String.class);
- getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConf");
- getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConfigurable");
- } catch (Exception e) {
- log.error("Error pre-loading JobConf and JobConfigurable classes, VFS classloader with "
- + "system classes in HDFS may not work correctly", e);
- System.exit(1);
- }
-
- if (args.length == 0) {
- printUsage();
- System.exit(1);
- }
- if (args[0].equals("-h") || args[0].equals("-help") || args[0].equals("--help")) {
- printUsage();
- System.exit(1);
- }
+ @SuppressWarnings("deprecation")
+ var deprecatedConfClass = org.apache.accumulo.start.classloader.AccumuloClassLoader
+ .getClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
+ confClass = deprecatedConfClass;
+ } catch (ClassNotFoundException e) {
+ log.error("Unable to find Hadoop Configuration class on classpath, check configuration.", e);
+ throw e;
+ }
+ Object conf = null;
+ try {
+ conf = confClass.getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ log.error("Error creating new instance of Hadoop Configuration", e);
+ throw e;
+ }
+ try {
+ Method getClassByNameOrNullMethod =
+ conf.getClass().getMethod("getClassByNameOrNull", String.class);
+ getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConf");
+ getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConfigurable");
+ } catch (Exception e) {
+ log.error("Error pre-loading JobConf and JobConfigurable classes, VFS classloader with "
+ + "system classes in HDFS may not work correctly", e);
+ throw e;
+ }
- // determine whether a keyword was used or a class name, and execute it with the remaining
- // args
- String keywordOrClassName = args[0];
- KeywordExecutable keywordExec = getExecutables(loader).get(keywordOrClassName);
- if (keywordExec != null) {
- execKeyword(keywordExec, stripArgs(args, 1));
- } else {
- execMainClassName(keywordOrClassName, stripArgs(args, 1));
- }
+ if (args.length == 0) {
+ printUsage();
+ throw new IllegalArgumentException("No arguments passed to Main");
Review comment:
This one might actually be a legitimate use for `System.exit(1)`, since the expectation here is that it scripts or users should see a non-zero return code, and that our usage information that we print is more useful than any ugly stack trace.
----------------------------------------------------------------
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
[GitHub] [accumulo] dlmarion commented on a change in pull request #1822: Closes #1349 - Throw errors from Main instead of using System.exit()
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #1822:
URL: https://github.com/apache/accumulo/pull/1822#discussion_r542788902
##########
File path: start/src/main/java/org/apache/accumulo/start/Main.java
##########
@@ -41,63 +41,57 @@
private static Class<?> vfsClassLoader;
private static Map<String,KeywordExecutable> servicesMap;
- public static void main(final String[] args) {
+ public static void main(final String[] args) throws Exception {
+ // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when
+ // using the VFSClassLoader with jars in HDFS.
+ ClassLoader loader = getClassLoader();
+ Class<?> confClass = null;
try {
- // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when
- // using the VFSClassLoader with jars in HDFS.
- ClassLoader loader = getClassLoader();
- Class<?> confClass = null;
- try {
- @SuppressWarnings("deprecation")
- var deprecatedConfClass = org.apache.accumulo.start.classloader.AccumuloClassLoader
- .getClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
- confClass = deprecatedConfClass;
- } catch (ClassNotFoundException e) {
- log.error("Unable to find Hadoop Configuration class on classpath, check configuration.",
- e);
- System.exit(1);
- }
- Object conf = null;
- try {
- conf = confClass.getDeclaredConstructor().newInstance();
- } catch (Exception e) {
- log.error("Error creating new instance of Hadoop Configuration", e);
- System.exit(1);
- }
- try {
- Method getClassByNameOrNullMethod =
- conf.getClass().getMethod("getClassByNameOrNull", String.class);
- getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConf");
- getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConfigurable");
- } catch (Exception e) {
- log.error("Error pre-loading JobConf and JobConfigurable classes, VFS classloader with "
- + "system classes in HDFS may not work correctly", e);
- System.exit(1);
- }
-
- if (args.length == 0) {
- printUsage();
- System.exit(1);
- }
- if (args[0].equals("-h") || args[0].equals("-help") || args[0].equals("--help")) {
- printUsage();
- System.exit(1);
- }
-
- // determine whether a keyword was used or a class name, and execute it with the remaining
- // args
- String keywordOrClassName = args[0];
- KeywordExecutable keywordExec = getExecutables(loader).get(keywordOrClassName);
- if (keywordExec != null) {
- execKeyword(keywordExec, stripArgs(args, 1));
- } else {
- execMainClassName(keywordOrClassName, stripArgs(args, 1));
- }
+ @SuppressWarnings("deprecation")
+ var deprecatedConfClass = org.apache.accumulo.start.classloader.AccumuloClassLoader
+ .getClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
+ confClass = deprecatedConfClass;
+ } catch (ClassNotFoundException e) {
+ log.error("Unable to find Hadoop Configuration class on classpath, check configuration.", e);
+ throw e;
+ }
+ Object conf = null;
+ try {
+ conf = confClass.getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ log.error("Error creating new instance of Hadoop Configuration", e);
+ throw e;
+ }
+ try {
+ Method getClassByNameOrNullMethod =
+ conf.getClass().getMethod("getClassByNameOrNull", String.class);
+ getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConf");
+ getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConfigurable");
+ } catch (Exception e) {
+ log.error("Error pre-loading JobConf and JobConfigurable classes, VFS classloader with "
+ + "system classes in HDFS may not work correctly", e);
+ throw e;
+ }
- } catch (Throwable t) {
- log.error("Uncaught exception", t);
+ if (args.length == 0) {
+ printUsage();
System.exit(1);
Review comment:
I'm fine with it as is.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1822: Closes #1349 - Throw errors from Main instead of using System.exit()
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1822:
URL: https://github.com/apache/accumulo/pull/1822#discussion_r542764972
##########
File path: start/src/main/java/org/apache/accumulo/start/Main.java
##########
@@ -41,63 +41,57 @@
private static Class<?> vfsClassLoader;
private static Map<String,KeywordExecutable> servicesMap;
- public static void main(final String[] args) {
+ public static void main(final String[] args) throws Exception {
+ // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when
+ // using the VFSClassLoader with jars in HDFS.
+ ClassLoader loader = getClassLoader();
+ Class<?> confClass = null;
try {
- // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when
- // using the VFSClassLoader with jars in HDFS.
- ClassLoader loader = getClassLoader();
- Class<?> confClass = null;
- try {
- @SuppressWarnings("deprecation")
- var deprecatedConfClass = org.apache.accumulo.start.classloader.AccumuloClassLoader
- .getClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
- confClass = deprecatedConfClass;
- } catch (ClassNotFoundException e) {
- log.error("Unable to find Hadoop Configuration class on classpath, check configuration.",
- e);
- System.exit(1);
- }
- Object conf = null;
- try {
- conf = confClass.getDeclaredConstructor().newInstance();
- } catch (Exception e) {
- log.error("Error creating new instance of Hadoop Configuration", e);
- System.exit(1);
- }
- try {
- Method getClassByNameOrNullMethod =
- conf.getClass().getMethod("getClassByNameOrNull", String.class);
- getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConf");
- getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConfigurable");
- } catch (Exception e) {
- log.error("Error pre-loading JobConf and JobConfigurable classes, VFS classloader with "
- + "system classes in HDFS may not work correctly", e);
- System.exit(1);
- }
-
- if (args.length == 0) {
- printUsage();
- System.exit(1);
- }
- if (args[0].equals("-h") || args[0].equals("-help") || args[0].equals("--help")) {
- printUsage();
- System.exit(1);
- }
-
- // determine whether a keyword was used or a class name, and execute it with the remaining
- // args
- String keywordOrClassName = args[0];
- KeywordExecutable keywordExec = getExecutables(loader).get(keywordOrClassName);
- if (keywordExec != null) {
- execKeyword(keywordExec, stripArgs(args, 1));
- } else {
- execMainClassName(keywordOrClassName, stripArgs(args, 1));
- }
+ @SuppressWarnings("deprecation")
+ var deprecatedConfClass = org.apache.accumulo.start.classloader.AccumuloClassLoader
+ .getClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
+ confClass = deprecatedConfClass;
+ } catch (ClassNotFoundException e) {
+ log.error("Unable to find Hadoop Configuration class on classpath, check configuration.", e);
+ throw e;
+ }
+ Object conf = null;
+ try {
+ conf = confClass.getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ log.error("Error creating new instance of Hadoop Configuration", e);
+ throw e;
+ }
+ try {
+ Method getClassByNameOrNullMethod =
+ conf.getClass().getMethod("getClassByNameOrNull", String.class);
+ getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConf");
+ getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConfigurable");
+ } catch (Exception e) {
+ log.error("Error pre-loading JobConf and JobConfigurable classes, VFS classloader with "
+ + "system classes in HDFS may not work correctly", e);
+ throw e;
+ }
- } catch (Throwable t) {
- log.error("Uncaught exception", t);
+ if (args.length == 0) {
+ printUsage();
System.exit(1);
Review comment:
@dlmarion Not sure if you saw this comment. I made it after marking "Approve" on my last review. Feel free to mark this as "resolved" if you're fine with it as-is.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1822: Closes #1349 - Throw errors from Main instead of using System.exit()
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1822:
URL: https://github.com/apache/accumulo/pull/1822#discussion_r542755567
##########
File path: start/src/main/java/org/apache/accumulo/start/Main.java
##########
@@ -41,63 +41,57 @@
private static Class<?> vfsClassLoader;
private static Map<String,KeywordExecutable> servicesMap;
- public static void main(final String[] args) {
+ public static void main(final String[] args) throws Exception {
+ // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when
+ // using the VFSClassLoader with jars in HDFS.
+ ClassLoader loader = getClassLoader();
+ Class<?> confClass = null;
try {
- // Preload classes that cause a deadlock between the ServiceLoader and the DFSClient when
- // using the VFSClassLoader with jars in HDFS.
- ClassLoader loader = getClassLoader();
- Class<?> confClass = null;
- try {
- @SuppressWarnings("deprecation")
- var deprecatedConfClass = org.apache.accumulo.start.classloader.AccumuloClassLoader
- .getClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
- confClass = deprecatedConfClass;
- } catch (ClassNotFoundException e) {
- log.error("Unable to find Hadoop Configuration class on classpath, check configuration.",
- e);
- System.exit(1);
- }
- Object conf = null;
- try {
- conf = confClass.getDeclaredConstructor().newInstance();
- } catch (Exception e) {
- log.error("Error creating new instance of Hadoop Configuration", e);
- System.exit(1);
- }
- try {
- Method getClassByNameOrNullMethod =
- conf.getClass().getMethod("getClassByNameOrNull", String.class);
- getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConf");
- getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConfigurable");
- } catch (Exception e) {
- log.error("Error pre-loading JobConf and JobConfigurable classes, VFS classloader with "
- + "system classes in HDFS may not work correctly", e);
- System.exit(1);
- }
-
- if (args.length == 0) {
- printUsage();
- System.exit(1);
- }
- if (args[0].equals("-h") || args[0].equals("-help") || args[0].equals("--help")) {
- printUsage();
- System.exit(1);
- }
-
- // determine whether a keyword was used or a class name, and execute it with the remaining
- // args
- String keywordOrClassName = args[0];
- KeywordExecutable keywordExec = getExecutables(loader).get(keywordOrClassName);
- if (keywordExec != null) {
- execKeyword(keywordExec, stripArgs(args, 1));
- } else {
- execMainClassName(keywordOrClassName, stripArgs(args, 1));
- }
+ @SuppressWarnings("deprecation")
+ var deprecatedConfClass = org.apache.accumulo.start.classloader.AccumuloClassLoader
+ .getClassLoader().loadClass("org.apache.hadoop.conf.Configuration");
+ confClass = deprecatedConfClass;
+ } catch (ClassNotFoundException e) {
+ log.error("Unable to find Hadoop Configuration class on classpath, check configuration.", e);
+ throw e;
+ }
+ Object conf = null;
+ try {
+ conf = confClass.getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ log.error("Error creating new instance of Hadoop Configuration", e);
+ throw e;
+ }
+ try {
+ Method getClassByNameOrNullMethod =
+ conf.getClass().getMethod("getClassByNameOrNull", String.class);
+ getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConf");
+ getClassByNameOrNullMethod.invoke(conf, "org.apache.hadoop.mapred.JobConfigurable");
+ } catch (Exception e) {
+ log.error("Error pre-loading JobConf and JobConfigurable classes, VFS classloader with "
+ + "system classes in HDFS may not work correctly", e);
+ throw e;
+ }
- } catch (Throwable t) {
- log.error("Uncaught exception", t);
+ if (args.length == 0) {
+ printUsage();
System.exit(1);
Review comment:
Even though this is a legitimate use of exit, if we want consistency with the help options, we could just return without an exit code here as well. I'm fine either way, as long as we don't throw an exception here.
----------------------------------------------------------------
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
[GitHub] [accumulo] dlmarion merged pull request #1822: Closes #1349 - Throw errors from Main instead of using System.exit()
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #1822:
URL: https://github.com/apache/accumulo/pull/1822
----------------------------------------------------------------
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