Copilot commented on code in PR #14517:
URL: https://github.com/apache/gravitino/pull/14517#discussion_r4439737231
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java:
##########
@@ -19,86 +19,280 @@
package org.apache.gravitino.catalog.glue;
import static
org.apache.gravitino.catalog.glue.GlueConstants.AWS_ACCESS_KEY_ID;
+import static
org.apache.gravitino.catalog.glue.GlueConstants.AWS_GLUE_API_CALL_ATTEMPT_TIMEOUT_MS;
+import static
org.apache.gravitino.catalog.glue.GlueConstants.AWS_GLUE_API_CALL_TIMEOUT_MS;
import static
org.apache.gravitino.catalog.glue.GlueConstants.AWS_GLUE_CATALOG_ID;
import static
org.apache.gravitino.catalog.glue.GlueConstants.AWS_GLUE_ENDPOINT;
+import static
org.apache.gravitino.catalog.glue.GlueConstants.AWS_GLUE_MAX_ATTEMPTS;
import static org.apache.gravitino.catalog.glue.GlueConstants.AWS_REGION;
import static
org.apache.gravitino.catalog.glue.GlueConstants.AWS_SECRET_ACCESS_KEY;
+import static
org.apache.gravitino.catalog.glue.GlueConstants.DEFAULT_AWS_GLUE_API_CALL_ATTEMPT_TIMEOUT_MS;
+import static
org.apache.gravitino.catalog.glue.GlueConstants.DEFAULT_AWS_GLUE_API_CALL_TIMEOUT_MS;
+import static
org.apache.gravitino.catalog.glue.GlueConstants.DEFAULT_AWS_GLUE_MAX_ATTEMPTS;
import static
org.apache.gravitino.catalog.glue.GlueConstants.DEFAULT_TABLE_FORMAT;
import static
org.apache.gravitino.catalog.glue.GlueConstants.DEFAULT_TABLE_FORMAT_FILTER;
import static
org.apache.gravitino.catalog.glue.GlueConstants.DEFAULT_TABLE_FORMAT_VALUE;
import static
org.apache.gravitino.catalog.glue.GlueConstants.TABLE_FORMAT_FILTER;
import static org.apache.gravitino.catalog.glue.GlueConstants.WAREHOUSE;
-import static
org.apache.gravitino.connector.PropertyEntry.stringOptionalPropertyEntry;
-import static
org.apache.gravitino.connector.PropertyEntry.stringRequiredPropertyEntry;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Arrays;
+import java.util.Locale;
import java.util.Map;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.regex.Pattern;
+import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.cloud.storage.AWSPropertiesMetadata;
import org.apache.gravitino.connector.BaseCatalogPropertiesMetadata;
import org.apache.gravitino.connector.PropertyEntry;
+import software.amazon.awssdk.regions.Region;
/** Properties metadata for the AWS Glue Data Catalog connector catalog-level
configuration. */
public class GlueCatalogPropertiesMetadata extends
BaseCatalogPropertiesMetadata {
+ /** Accepted values of {@code default-table-format}, compared
case-insensitively. */
+ static final Set<String> DEFAULT_TABLE_FORMATS = ImmutableSet.of("iceberg",
"hive");
+
+ /** Accepted entries of {@code table-format-filter}, compared
case-insensitively. */
+ static final Set<String> TABLE_FORMAT_FILTER_VALUES =
+ ImmutableSet.of(DEFAULT_TABLE_FORMAT_FILTER, "hive", "iceberg", "delta",
"parquet");
+
+ private static final Set<String> WAREHOUSE_SCHEMES = ImmutableSet.of("s3",
"s3a", "s3n");
+
+ private static final Pattern CATALOG_ID_PATTERN = Pattern.compile("\\d{12}");
+
private static final Map<String, PropertyEntry<?>> PROPERTIES_METADATA =
ImmutableMap.<String, PropertyEntry<?>>builder()
.put(
AWS_REGION,
- stringRequiredPropertyEntry(
+ stringEntry(
AWS_REGION,
"AWS region for the Glue Data Catalog (e.g. us-east-1)",
+ true /* required */,
true /* immutable */,
- false /* hidden */))
+ null /* defaultValue */,
+ GlueCatalogPropertiesMetadata::checkRegion))
.put(
AWS_GLUE_CATALOG_ID,
- stringOptionalPropertyEntry(
+ stringEntry(
AWS_GLUE_CATALOG_ID,
"The 12-digit AWS account ID that owns the Glue catalog."
+ " When omitted, defaults to the caller's AWS account
ID.",
+ false /* required */,
true /* immutable */,
null /* defaultValue */,
- false /* hidden */))
+ GlueCatalogPropertiesMetadata::checkCatalogId))
.put(AWS_ACCESS_KEY_ID, AWSPropertiesMetadata.AWS_ACCESS_KEY_ID)
.put(AWS_SECRET_ACCESS_KEY,
AWSPropertiesMetadata.AWS_SECRET_ACCESS_KEY)
.put(
AWS_GLUE_ENDPOINT,
- stringOptionalPropertyEntry(
+ stringEntry(
AWS_GLUE_ENDPOINT,
"Custom Glue endpoint URL for VPC endpoints or LocalStack
testing"
+ " (e.g. http://localhost:4566)",
+ false /* required */,
false /* immutable */,
null /* defaultValue */,
- false /* hidden */))
+ GlueCatalogPropertiesMetadata::checkEndpoint))
+ .put(
+ AWS_GLUE_API_CALL_TIMEOUT_MS,
+ positiveLongEntry(
+ AWS_GLUE_API_CALL_TIMEOUT_MS,
+ "Upper bound in milliseconds for one Glue API call,
including retries.",
+ DEFAULT_AWS_GLUE_API_CALL_TIMEOUT_MS))
+ .put(
+ AWS_GLUE_API_CALL_ATTEMPT_TIMEOUT_MS,
+ positiveLongEntry(
+ AWS_GLUE_API_CALL_ATTEMPT_TIMEOUT_MS,
+ "Upper bound in milliseconds for a single attempt of a Glue
API call.",
+ DEFAULT_AWS_GLUE_API_CALL_ATTEMPT_TIMEOUT_MS))
+ .put(
+ AWS_GLUE_MAX_ATTEMPTS,
+ new PropertyEntry.Builder<Integer>()
+ .withName(AWS_GLUE_MAX_ATTEMPTS)
+ .withDescription(
+ "Maximum number of attempts of a Glue API call,
including the first one.")
+ .withJavaType(Integer.class)
+ .withDefaultValue(DEFAULT_AWS_GLUE_MAX_ATTEMPTS)
+ .withDecoder(
+ value -> {
+ int attempts = Integer.parseInt(value.trim());
+ Preconditions.checkArgument(attempts > 0, "must be
positive");
+ return attempts;
+ })
+ .withEncoder(String::valueOf)
+ .build())
.put(
DEFAULT_TABLE_FORMAT,
- stringOptionalPropertyEntry(
+ stringEntry(
DEFAULT_TABLE_FORMAT,
"Default format for tables created via createTable().
Accepted: iceberg, hive.",
+ false /* required */,
false /* immutable */,
DEFAULT_TABLE_FORMAT_VALUE,
- false /* hidden */))
+ GlueCatalogPropertiesMetadata::checkDefaultTableFormat))
.put(
TABLE_FORMAT_FILTER,
- stringOptionalPropertyEntry(
+ stringEntry(
TABLE_FORMAT_FILTER,
- "Comma-separated table formats exposed by listTables() and
loadTable().",
+ "Comma-separated table formats exposed by listTables() and
loadTable()."
+ + " Accepted: all, hive, iceberg, delta, parquet.",
+ false /* required */,
false /* immutable */,
DEFAULT_TABLE_FORMAT_FILTER,
- false /* hidden */))
+ GlueCatalogPropertiesMetadata::checkTableFormatFilter))
.put(
WAREHOUSE,
- stringRequiredPropertyEntry(
+ stringEntry(
WAREHOUSE,
"Base storage path used as warehouse when no explicit
location is set"
+ " at table creation time and the database declares no
LocationUri"
+ " (e.g. s3://my-bucket/warehouse)."
+ " Table location is then derived as
warehouse/database/table.",
+ true /* required */,
false /* immutable */,
- false /* hidden */))
+ null /* defaultValue */,
+ GlueCatalogPropertiesMetadata::checkWarehouse))
.build();
@Override
protected Map<String, PropertyEntry<?>> specificPropertyEntries() {
return PROPERTIES_METADATA;
}
+
+ /**
+ * Parses {@code aws-glue-endpoint} into a URI with an {@code http} or
{@code https} scheme and a
+ * host.
+ *
+ * @param endpoint the configured endpoint
+ * @return the parsed endpoint URI
+ * @throws IllegalArgumentException if the endpoint is not such a URI
+ */
+ static URI parseEndpoint(String endpoint) {
+ // The authority, not getHost(), is checked: getHost() is null for host
names with '_', such
+ // as Docker service names.
+ URI uri;
+ try {
+ uri = new URI(endpoint);
+ } catch (URISyntaxException e) {
+ throw new IllegalArgumentException(
+ String.format("Property '%s' is not a valid URI: %s",
AWS_GLUE_ENDPOINT, e.getMessage()),
+ e);
+ }
+ Preconditions.checkArgument(
+ ("http".equalsIgnoreCase(uri.getScheme()) ||
"https".equalsIgnoreCase(uri.getScheme()))
+ && StringUtils.isNotBlank(uri.getRawAuthority()),
Review Comment:
A nonempty authority does not guarantee a host: both `http://:4566` and
`http://user@` pass this check with no hostname. These unusable endpoints
therefore pass both property validation and validateOnCreate and can be
persisted, only failing when the catalog is used. Validate that the authority
contains a nonempty hostname and a valid port while retaining the intended
support for underscore-containing Docker names.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -174,46 +313,114 @@ 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, which reports Glue and S3
errors either as its
+ * own exception types or wrapped around the AWS SDK exception, into the
same Gravitino exceptions
+ * the Hive path returns. Exceptions that are already Gravitino or argument
errors 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;
+ }
+ for (Throwable t = e; t != null; t = t.getCause() == t ? null :
t.getCause()) {
+ 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, stripCatalogName(t.getMessage()));
+ }
+ if (t instanceof org.apache.iceberg.exceptions.NoSuchNamespaceException)
{
+ return new NoSuchSchemaException(e, "%s: %s", context,
stripCatalogName(t.getMessage()));
+ }
+ if (t instanceof org.apache.iceberg.exceptions.NoSuchTableException) {
+ return new NoSuchTableException(e, "%s: %s", context,
stripCatalogName(t.getMessage()));
+ }
+ if (t instanceof org.apache.iceberg.exceptions.ValidationException) {
+ return new IllegalArgumentException(
+ String.format("Invalid request for %s: %s", context,
stripCatalogName(t.getMessage())),
+ e);
+ }
+ if (t instanceof SdkException) {
+ return toGenericException((SdkException) t, context, null);
Review Comment:
A raw or wrapped Glue `EntityNotFoundException` reaches
`toGenericException()`, which returns a plain `RuntimeException` instead of a
not-found error. When Iceberg creation with an explicit location reports a
missing database through this AWS exception, the response therefore remains
HTTP 500. Handle AWS not-found errors before the generic fallback, using
operation context to distinguish a missing schema during creation from a
missing table during alteration.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -133,11 +182,110 @@ static ConnectionFailedException
toConnectionException(SdkException e) {
awsErrorDetail((GlueException) e));
}
+ String detail =
+ e instanceof GlueException
+ ? awsErrorDetail((GlueException) e)
+ : StringUtils.defaultIfBlank(
+ ExceptionMessages.usefulMessage(e),
e.getClass().getSimpleName());
+ return new ConnectionFailedException(
+ e,
+ "Failed to connect to AWS Glue at %s: %s",
+ GlueClientProvider.describeEndpoint(client),
+ detail);
+ }
+
+ /**
+ * Converts the failures that do not depend on the object being operated on:
missing, rejected or
+ * mis-scoped credentials, throttling, invalid input, access denial and an
unreachable endpoint.
+ *
+ * @param e the SDK failure
+ * @param context description of the operation context for error messages
+ * @param client the Glue client that made the call, used to name its region
and endpoint; may be
+ * null
+ * @return the converted exception, or null when the failure is specific to
the object (for
+ * example not found or already exists) and must be converted by the
caller
+ */
+ @Nullable
+ static RuntimeException toClientException(
+ SdkException e, String context, @Nullable GlueClient client) {
+ if (e instanceof SdkClientException) {
+ SdkClientException clientException = (SdkClientException) e;
+ if (isCredentialFailure(clientException)) {
+ return toCredentialException(clientException, context);
+ }
+ String endpoint = GlueClientProvider.describeEndpoint(client);
+ String detail =
+ StringUtils.defaultIfBlank(
+ ExceptionMessages.usefulMessage(e),
e.getClass().getSimpleName());
+ if (e instanceof ApiCallTimeoutException || e instanceof
ApiCallAttemptTimeoutException) {
+ return new ConnectionFailedException(
+ e, "AWS Glue at %s timed out for %s: %s", endpoint, context,
detail);
+ }
+ return new ConnectionFailedException(
+ e, "Failed to reach AWS Glue at %s for %s: %s", endpoint, context,
detail);
+ }
+ if (!(e instanceof GlueException)) {
+ return null;
Review Comment:
Iceberg writes metadata through `S3FileIO`, but this Glue-only guard leaves
an S3 `AccessDenied` response mapped to a plain `RuntimeException` and HTTP 500
rather than 403. The same fallback can downgrade an Iceberg
`ForbiddenException` when its SDK cause is an S3 exception. Map S3
authorization errors by AWS code/status, preserve the existing forbidden
classification when its cause has no recognized mapping, and add an
`S3Exception` regression test.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueIcebergTableHelper.java:
##########
@@ -188,6 +196,13 @@ static Map<String, String>
buildIcebergCatalogProperties(Map<String, String> con
icebergProps.put(GLUE_ENDPOINT, endpoint);
}
+ // The Iceberg AWS clients take no total call timeout, so bound each
connection and read by
+ // the per-attempt timeout of the catalog's own Glue client.
+ String attemptTimeoutMs =
String.valueOf(GlueClientProvider.apiCallAttemptTimeoutMs(config));
+ icebergProps.put(HTTP_CLIENT_TYPE, "apache");
+ icebergProps.put(HTTP_CLIENT_APACHE_CONNECTION_TIMEOUT_MS,
attemptTimeoutMs);
+ icebergProps.put(HTTP_CLIENT_APACHE_SOCKET_TIMEOUT_MS, attemptTimeoutMs);
Review Comment:
The Iceberg client receives only connection/socket timeouts, so
`aws-glue-api-call-timeout-ms` and `aws-glue-max-attempts` do not constrain its
Glue calls. Iceberg create/alter requests can therefore exceed the configured
total timeout and retry limit, and socket timeouts do not enforce a per-attempt
deadline. Apply the same SDK call timeout, attempt timeout and retry limit to
Iceberg's underlying Glue client through a configured AWS client factory.
--
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]
