You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "eric-maynard (via GitHub)" <gi...@apache.org> on 2025/04/09 21:39:13 UTC
[PR] Surface failed BaseResults to the caller more often [polaris]
eric-maynard opened a new pull request, #1348:
URL: https://github.com/apache/polaris/pull/1348
In many `...Catalog` methods like `dropPolicy` or `dropGenericTable`, we simply return `false` if there's a `BaseResult` from persistence that doesn't have a `SUCCESS` error code. In some cases like in `IcebergCatalog` we do a little better, mapping a small subset of failures into exceptions.
This PR pushes this mapping down into `BaseResult` so that methods which rely on `BaseResult` to report the success of a persistence call can pass accurate information about the error back to the caller (via the exception mapper in the case of a REST call).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Surface persistence exceptions to the caller more often [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1348:
URL: https://github.com/apache/polaris/pull/1348#discussion_r2036256675
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java:
##########
@@ -57,14 +70,90 @@ public String getExtraInformation() {
return extraInformation;
}
- public boolean isSuccess() {
+ public final boolean isSuccess() {
return this.returnStatusCode == ReturnStatus.SUCCESS.getCode();
}
- public boolean alreadyExists() {
+ public final boolean alreadyExists() {
return this.returnStatusCode == ReturnStatus.ENTITY_ALREADY_EXISTS.getCode();
}
+ /**
+ * If this result is not a successful one, this builds an exception from the failed result which
+ * the exception mapper can use to provide the caller some useful information about the failure.
+ * The message relies on `extraInformation`. If {@link
+ * org.apache.polaris.core.config.FeatureConfiguration#DETAILED_PERSISTENCE_EXCEPTIONS} is false,
+ * this should return an empty option.
+ */
+ @SuppressWarnings("FormatStringAnnotation")
+ public Optional<RuntimeException> getException(CallContext callContext) {
+ if (!callContext
+ .getPolarisCallContext()
+ .getConfigurationStore()
+ .getConfiguration(
+ callContext.getPolarisCallContext(),
+ FeatureConfiguration.DETAILED_PERSISTENCE_EXCEPTIONS)) {
+ return Optional.empty();
+ }
+
+ String message = this.extraInformation;
+ if (this.extraInformation == null) {
+ // TODO this should ideally never be hit but it's hit often.
Review Comment:
If we merge this, we should file an issue(s) tracking the many callsites where a non-successful result is built with a `null` message. This might be a good starter task.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Surface persistence exceptions to the caller more often [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard closed pull request #1348: Surface persistence exceptions to the caller more often
URL: https://github.com/apache/polaris/pull/1348
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Surface failed BaseResults to the caller more often [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #1348:
URL: https://github.com/apache/polaris/pull/1348#issuecomment-2791073080
Unfortunately some Iceberg tests essentially assert that these methods don't throw exceptions, e.g. `testDropMissingTable` does:
```
((AbstractBooleanAssert)Assertions.assertThat(catalog.dropTable(noSuchTableIdent)).as("Should not drop a table that does not exist", new Object[0])).isFalse();
```
To pass these tests, we may want to gate this new behavior behind a flag
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Surface persistence exceptions to the caller more often [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1348:
URL: https://github.com/apache/polaris/pull/1348#issuecomment-2868221755
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Surface failed BaseResults to the caller more often [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1348:
URL: https://github.com/apache/polaris/pull/1348#discussion_r2036178512
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/BaseResult.java:
##########
@@ -57,14 +64,67 @@ public String getExtraInformation() {
return extraInformation;
}
- public boolean isSuccess() {
+ public final boolean isSuccess() {
return this.returnStatusCode == ReturnStatus.SUCCESS.getCode();
}
- public boolean alreadyExists() {
+ public final boolean alreadyExists() {
return this.returnStatusCode == ReturnStatus.ENTITY_ALREADY_EXISTS.getCode();
}
+ /**
+ * If this result is not a successful one, this builds an exception from the failed result which
+ * the exception mapper can use to provide the caller some useful information about the failure.
+ * The message relies on `extraInformation`.
+ */
+ @SuppressWarnings("FormatStringAnnotation")
+ public Optional<RuntimeException> getException() {
+ // TODO use a switch expression if the Java version here is ever raised
+ if (this.returnStatusCode == ReturnStatus.SUCCESS.getCode()) {
+ return Optional.empty();
+ } else if (this.returnStatusCode == ReturnStatus.UNEXPECTED_ERROR_SIGNALED.getCode()) {
+ return Optional.of(new RuntimeException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.CATALOG_PATH_CANNOT_BE_RESOLVED.getCode()) {
+ return Optional.of(new NotFoundException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RESOLVED.getCode()) {
+ return Optional.of(new NotFoundException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.ENTITY_NOT_FOUND.getCode()) {
+ return Optional.of(new NotFoundException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.GRANT_NOT_FOUND.getCode()) {
+ return Optional.of(new NotFoundException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.ENTITY_ALREADY_EXISTS.getCode()) {
+ return Optional.of(new AlreadyExistsException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.NAMESPACE_NOT_EMPTY.getCode()) {
+ return Optional.of(new NamespaceNotEmptyException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.CATALOG_NOT_EMPTY.getCode()) {
+ return Optional.of(new BadRequestException(this.extraInformation));
+ } else if (this.returnStatusCode
+ == ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED.getCode()) {
+ return Optional.of(new CommitFailedException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.ENTITY_CANNOT_BE_RENAMED.getCode()) {
+ return Optional.of(new BadRequestException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.SUBSCOPE_CREDS_ERROR.getCode()) {
+ return Optional.of(new UnprocessableEntityException(this.extraInformation));
+ } else if (this.returnStatusCode == ReturnStatus.POLICY_MAPPING_NOT_FOUND.getCode()) {
+ return Optional.of(new NotFoundException(this.extraInformation));
+ } else if (this.returnStatusCode
+ == ReturnStatus.POLICY_MAPPING_OF_SAME_TYPE_ALREADY_EXISTS.getCode()) {
+ return Optional.of(new AlreadyExistsException(this.extraInformation));
+ } else {
+ return Optional.of(new RuntimeException(this.extraInformation));
+ }
+ }
+
+ /**
+ * If this result is failed, this should throw the appropriate exception which corresponds to the
+ * result status. See {@link BaseResult#getException} for details.
+ */
+ public void maybeThrowException() throws RuntimeException {
Review Comment:
This helper method is nice to have because you can't throw an exception from a lambda, so you can't do e.g. `result.getException().foreach(e -> throw 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.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Surface persistence exceptions to the caller more often [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1348:
URL: https://github.com/apache/polaris/pull/1348#issuecomment-2982375058
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org