Copilot commented on code in PR #14565:
URL: https://github.com/apache/gravitino/pull/14565#discussion_r4452443879
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -174,46 +329,179 @@ static RuntimeException toSchemaException(GlueException
e, String context) {
* @return a Gravitino or standard Java runtime exception
*/
static RuntimeException toTableException(GlueException e, String context) {
- if (isAuthenticationFailure(e)) {
- return toAuthenticationException(e, context);
- }
if (e instanceof EntityNotFoundException) {
return new NoSuchTableException(e, "%s does not exist", context);
}
if (e instanceof AlreadyExistsException) {
return new TableAlreadyExistsException(e, "%s already exists", context);
}
- if (e instanceof InvalidInputException) {
- return ExceptionMessages.illegalArgument(context, e);
+ return toGenericException(e, context, null);
+ }
+
+ /**
+ * Converts a failure raised by the Iceberg SDK into a Gravitino exception.
Iceberg reports Glue
+ * and S3 errors either as its own exception types or wrapped around the AWS
SDK exception, so the
+ * cause chain is walked from the outside in and the first recognized type
decides the result:
+ * Iceberg not-found, already-exists, validation and forbidden errors map to
the matching
+ * Gravitino types, a commit conflict maps to {@link
OptimisticLockException}, S3 failures map by
+ * their HTTP status, and other AWS SDK causes go through {@link
#toGenericException}. A commit
+ * whose state is unknown is reported as such, because the change may have
been applied. {@link
+ * GravitinoRuntimeException}, {@link IllegalArgumentException} and {@link
+ * UnsupportedOperationException} are returned as is.
+ *
+ * @param e the failure raised by an Iceberg catalog call
+ * @param context description of the operation context for error messages
+ * @return a Gravitino or standard Java runtime exception
+ */
+ static RuntimeException toIcebergException(RuntimeException e, String
context) {
+ if (e instanceof GravitinoRuntimeException
+ || e instanceof IllegalArgumentException
+ || e instanceof UnsupportedOperationException) {
+ return e;
}
- if (e instanceof AccessDeniedException) {
- return new ForbiddenException(e, "Glue error: %s: %s", context,
awsErrorDetail(e));
+ for (Throwable t = e; t != null; t = t.getCause() == t ? null :
t.getCause()) {
+ if (t instanceof CommitStateUnknownException) {
+ // A local validation failure underneath means nothing was sent, so
the commit did not
+ // happen. Any other cause, such as a transport failure, does not mean
the commit failed.
+ Throwable invalid = findCause(t,
org.apache.iceberg.exceptions.ValidationException.class);
+ if (invalid != null) {
+ return new IllegalArgumentException(
+ String.format("Invalid request for %s: %s", context,
describe(invalid)), e);
+ }
+ return new RuntimeException(
+ String.format(
+ "%s: the commit state is unknown and the change may have been
applied; check the"
+ + " table before retrying: %s",
+ context, describe(t.getCause() != null ? t.getCause() : t)),
+ e);
+ }
+ if (t instanceof CommitFailedException) {
+ Throwable awsCause = findCause(t, SdkException.class);
+ return awsCause == null || awsCause instanceof
ConcurrentModificationException
+ ? new OptimisticLockException(
+ e, "%s: the table was changed concurrently, retry: %s",
context, describe(t))
+ : toGenericException((SdkException) awsCause, context, null);
+ }
+ if (t instanceof AlreadyExistsException) {
+ return new TableAlreadyExistsException(
+ e, "%s: %s", context, awsErrorDetail((AlreadyExistsException) t));
+ }
+ if (t instanceof org.apache.iceberg.exceptions.AlreadyExistsException) {
+ return new TableAlreadyExistsException(e, "%s: %s", context,
describe(t));
+ }
+ if (t instanceof NoSuchNamespaceException) {
+ return new NoSuchSchemaException(e, "%s: %s", context, describe(t));
+ }
+ if (t instanceof org.apache.iceberg.exceptions.NoSuchTableException) {
+ return new NoSuchTableException(e, "%s: %s", context, describe(t));
+ }
+ if (t instanceof org.apache.iceberg.exceptions.NotFoundException) {
+ // Iceberg wraps a Glue EntityNotFoundException here; it may be the
table or its database.
+ return new NotFoundException(e, "%s: %s", context, describe(t));
+ }
+ if (t instanceof org.apache.iceberg.exceptions.ValidationException) {
+ return new IllegalArgumentException(
+ String.format("Invalid request for %s: %s", context, describe(t)),
e);
+ }
+ if (t instanceof org.apache.iceberg.exceptions.ForbiddenException) {
+ // A Glue denial deeper in the chain keeps its AWS error code and
message through the
+ // shared mapping; a denial by another service (such as S3) stays
forbidden.
+ Throwable awsCause = findCause(t, SdkException.class);
+ return awsCause instanceof GlueException
+ ? toGenericException((SdkException) awsCause, context, null)
+ : new ForbiddenException(
+ e, "%s: %s", context, describe(awsCause != null ? awsCause :
t));
+ }
+ if (t instanceof EntityNotFoundException) {
+ // Not wrapped by Iceberg; it may be the table or its database.
+ return new NotFoundException(e, "%s: %s", context, describe(t));
+ }
+ if (t instanceof S3Exception) {
+ return toS3Exception((S3Exception) t, context, e);
+ }
+ if (t instanceof SdkException) {
+ return toGenericException((SdkException) t, context, null);
+ }
Review Comment:
An S3 metadata read that fails with an SDK transport error (for example, a
socket timeout) reaches this branch, but `toGenericException()` reports it as a
failure to reach AWS Glue. The underlying SDK client exception does not
identify Glue, so this sends operators to the wrong service when the S3 timeout
added in this PR fires. Handle client-side SDK failures here with a
service-neutral AWS/Iceberg message, preserving the credential-failure handling.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTable.java:
##########
@@ -96,8 +96,23 @@ protected TableOperations newOps() {
return new GlueTableOperations(glueClient, catalogId, dbName, name,
partColNames);
}
+ /**
+ * Returns the Glue partition operations of a Hive-format table. Iceberg
tables keep their
+ * partitions in Iceberg manifests, not in Glue, so they have none.
+ *
+ * @throws UnsupportedOperationException for an Iceberg-format table
+ */
@Override
public SupportsPartitions supportPartitions() throws
UnsupportedOperationException {
+ if (properties != null
+ && GlueConstants.ICEBERG_TABLE_TYPE_VALUE.equalsIgnoreCase(
+ properties.get(GlueConstants.TABLE_TYPE_PARAM))) {
Review Comment:
This checks the merged public properties rather than the format of the
stored Glue table. `GlueIcebergTableHelper.loadTable()` lets Iceberg metadata
properties overwrite Glue parameters, and `table_type` is not reserved, so
setting `table_type=HIVE` on an Iceberg table makes this guard pass and exposes
Glue partition operations again. Preserve the Iceberg-format flag from the raw
Glue table and use that flag here instead of a user-modifiable property.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
