You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/01/25 12:00:25 UTC
[GitHub] [iceberg] rymurr opened a new pull request #2146: Improved error messages in CatalogUtil.loadCatalog
rymurr opened a new pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146
closes #2145
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146#discussion_r564745063
##########
File path: common/src/main/java/org/apache/iceberg/common/DynConstructors.java
##########
@@ -235,6 +236,10 @@ public Builder hiddenImpl(String className, Class<?>... types) {
throw new RuntimeException("Cannot find constructor for " +
baseClass + "\n" + formatProblems(problems));
}
+
+ public Map<String, Throwable> problems() {
+ return ImmutableMap.copyOf(problems);
+ }
Review comment:
Why does this need to be public? The problems are already added when exceptions are thrown.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on a change in pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146#discussion_r565107182
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -129,6 +129,35 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Get constructor for Catalog from impl. Impl must be valid class name and have zero-arg constructor.
+ *
+ * <p>We try to get the constructor for impl and if we fail we check for common error patterns. Specifically:
+ * a ClassNotFoundException for an invalid impl String and a NoClassDefFoundError if impl fails to get initialized.
+ * Otherwise the original exception is passed up and the error is assumed to be a missing no-args constructor.
+ *
+ * @param impl catalog implementation full class name
+ * @return constructor for catalog implementation impl
+ * @throws IllegalArgumentException if no-arg constructor not found or error during initialization
+ */
+ private static DynConstructors.Ctor<Catalog> getCatalogConstructor(String impl) {
+ DynConstructors.Ctor<Catalog> ctor;
+ DynConstructors.Builder ctorBuilder = DynConstructors.builder(Catalog.class).impl(impl);
+ try {
+ ctor = ctorBuilder.buildChecked();
+ } catch (NoSuchMethodException e) {
+ Throwable originalError = ctorBuilder.problems().getOrDefault(impl, e);
+ String message = String.format("missing no-arg constructor: %s", impl);
+ if (originalError instanceof ClassNotFoundException) {
+ message = String.format("catalog class %s not found", impl);
+ } else if (originalError instanceof NoClassDefFoundError) {
+ message = String.format("catalog %s cannot be initialized", impl);
+ }
+ throw new IllegalArgumentException(String.format("Cannot initialize Catalog, %s", message), originalError);
+ }
Review comment:
You are of course right @rdblue, I got obsessed with tailoring a message to an exact exception class and lost perspective. I have updated with your recommendation for a much simpler change.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on a change in pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146#discussion_r564262364
##########
File path: core/src/test/java/org/apache/iceberg/TestCatalogUtil.java
##########
@@ -87,6 +87,32 @@ public void loadCustomCatalog_NotImplementCatalog() {
() -> CatalogUtil.loadCatalog(TestCatalogNoInterface.class.getName(), name, options, hadoopConf));
}
+ @Test
+ public void loadCustomCatalog_ConstructorErrorCatalog() {
+ Map<String, String> options = new HashMap<>();
+ options.put("key", "val");
+ Configuration hadoopConf = new Configuration();
+ String name = "custom";
+
+ String impl = TestCatalogErrorConstructor.class.getName();
+ AssertHelpers.assertThrows("must be able to initialize catalog",
Review comment:
all the other tests use this format 'must be able to', 'must have' etc.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146#issuecomment-769337943
Thanks, @rymurr! Looks great!
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yyanyy commented on a change in pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
yyanyy commented on a change in pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146#discussion_r564199952
##########
File path: core/src/test/java/org/apache/iceberg/TestCatalogUtil.java
##########
@@ -87,6 +87,32 @@ public void loadCustomCatalog_NotImplementCatalog() {
() -> CatalogUtil.loadCatalog(TestCatalogNoInterface.class.getName(), name, options, hadoopConf));
}
+ @Test
+ public void loadCustomCatalog_ConstructorErrorCatalog() {
+ Map<String, String> options = new HashMap<>();
+ options.put("key", "val");
+ Configuration hadoopConf = new Configuration();
+ String name = "custom";
+
+ String impl = TestCatalogErrorConstructor.class.getName();
+ AssertHelpers.assertThrows("must be able to initialize catalog",
Review comment:
Nit: "must not be able to"?
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -129,6 +129,35 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Get constructor for Catalog from impl. Impl must be valid class name and have zero-arg constructor.
+ *
+ * <p>We try to get the constructor for impl and if we fail we check for common error patterns. Specifically:
+ * a ClassNotFoundException for an invalid impl String and a NoClassDefFoundError if impl fails to get initialized.
+ * Otherwise the original exception is passed up and the error is assumed to be a missing no-args constructor.
+ *
+ * @param impl catalog implementation full class name
+ * @return constructor for catalog implementation impl
+ * @throws IllegalArgumentException if no-arg constructor not found or error during initialization
+ */
+ private static DynConstructors.Ctor<Catalog> getCatalogConstructor(String impl) {
+ DynConstructors.Ctor<Catalog> ctor;
+ DynConstructors.Builder ctorBuilder = DynConstructors.builder(Catalog.class).impl(impl);
+ try {
+ ctor = ctorBuilder.buildChecked();
+ } catch (NoSuchMethodException e) {
+ Throwable originalError = ctorBuilder.problems().getOrDefault(impl, e);
+ String message = String.format("Cannot initialize Catalog, missing no-arg constructor: %s", impl);
Review comment:
Nit: "Cannot initialize Catalog" will be printed twice if the error is neither of the one below. Although I wonder if there's a specific exception thrown if the catalog class can be found but does not have a no-arg constructor, and if so we probably could catch and print that instead, since I'd suspect the chance of misconfiguring could be much larger than only missing the no-arg constructor itself, and hence "cannot be initialized" might be a better default ...
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146#discussion_r564746337
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -129,6 +129,35 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Get constructor for Catalog from impl. Impl must be valid class name and have zero-arg constructor.
+ *
+ * <p>We try to get the constructor for impl and if we fail we check for common error patterns. Specifically:
+ * a ClassNotFoundException for an invalid impl String and a NoClassDefFoundError if impl fails to get initialized.
+ * Otherwise the original exception is passed up and the error is assumed to be a missing no-args constructor.
+ *
+ * @param impl catalog implementation full class name
+ * @return constructor for catalog implementation impl
+ * @throws IllegalArgumentException if no-arg constructor not found or error during initialization
+ */
+ private static DynConstructors.Ctor<Catalog> getCatalogConstructor(String impl) {
+ DynConstructors.Ctor<Catalog> ctor;
+ DynConstructors.Builder ctorBuilder = DynConstructors.builder(Catalog.class).impl(impl);
+ try {
+ ctor = ctorBuilder.buildChecked();
+ } catch (NoSuchMethodException e) {
+ Throwable originalError = ctorBuilder.problems().getOrDefault(impl, e);
+ String message = String.format("missing no-arg constructor: %s", impl);
+ if (originalError instanceof ClassNotFoundException) {
+ message = String.format("catalog class %s not found", impl);
+ } else if (originalError instanceof NoClassDefFoundError) {
+ message = String.format("catalog %s cannot be initialized", impl);
+ }
+ throw new IllegalArgumentException(String.format("Cannot initialize Catalog, %s", message), originalError);
+ }
Review comment:
I don't get why this doesn't use the error message with all of the problems listed. Wouldn't that be easier? The original `try` could be used, like this:
```java
try {
ctor = DynConstructors.builder(Catalog.class).impl(impl).buildChecked();
} catch (NoSuchMethodException e) {
throw new IllegalArgumentException(String.format(
"Cannot initialize Catalog implementation %s: %s", impl, e.getMessage()), e);
}
```
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146#discussion_r564744618
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -129,6 +129,35 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Get constructor for Catalog from impl. Impl must be valid class name and have zero-arg constructor.
+ *
+ * <p>We try to get the constructor for impl and if we fail we check for common error patterns. Specifically:
+ * a ClassNotFoundException for an invalid impl String and a NoClassDefFoundError if impl fails to get initialized.
+ * Otherwise the original exception is passed up and the error is assumed to be a missing no-args constructor.
+ *
+ * @param impl catalog implementation full class name
+ * @return constructor for catalog implementation impl
+ * @throws IllegalArgumentException if no-arg constructor not found or error during initialization
+ */
+ private static DynConstructors.Ctor<Catalog> getCatalogConstructor(String impl) {
Review comment:
Nit: use of `get` when a more descriptive verb would be better. For example, `find` implies that it may not be found.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on a change in pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146#discussion_r564261743
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -129,6 +129,35 @@ private static void deleteFiles(FileIO io, Set<ManifestFile> allManifests) {
});
}
+ /**
+ * Get constructor for Catalog from impl. Impl must be valid class name and have zero-arg constructor.
+ *
+ * <p>We try to get the constructor for impl and if we fail we check for common error patterns. Specifically:
+ * a ClassNotFoundException for an invalid impl String and a NoClassDefFoundError if impl fails to get initialized.
+ * Otherwise the original exception is passed up and the error is assumed to be a missing no-args constructor.
+ *
+ * @param impl catalog implementation full class name
+ * @return constructor for catalog implementation impl
+ * @throws IllegalArgumentException if no-arg constructor not found or error during initialization
+ */
+ private static DynConstructors.Ctor<Catalog> getCatalogConstructor(String impl) {
+ DynConstructors.Ctor<Catalog> ctor;
+ DynConstructors.Builder ctorBuilder = DynConstructors.builder(Catalog.class).impl(impl);
+ try {
+ ctor = ctorBuilder.buildChecked();
+ } catch (NoSuchMethodException e) {
+ Throwable originalError = ctorBuilder.problems().getOrDefault(impl, e);
+ String message = String.format("Cannot initialize Catalog, missing no-arg constructor: %s", impl);
Review comment:
I suspect no-arg constructor is likely to be the most common error. 'Cannot be initialized' is related to classpath issues only. I have updated to remove the dup message.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on pull request #2146: Improved error messages in CatalogUtil.loadCatalog
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #2146:
URL: https://github.com/apache/iceberg/pull/2146#issuecomment-767322165
Thanks for the feedback and approval @yyanyy I have updated to fix the dup message
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org