This is an automated email from the ASF dual-hosted git repository. diqiu50 pushed a commit to branch glue-pr03 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 17ebf39c480b4503b7c77a5808af9b2bd5c2fa0c Author: diqiu50 <[email protected]> AuthorDate: Fri Apr 10 20:23:31 2026 +0800 fix(catalog-glue): Address yuqi1129 review comments - Use StringUtils.isNotBlank() to reject blank region and credential values - Make aws-glue-catalog-id optional (Glue defaults to caller's account ID) - Add casing note to TABLE_FORMAT_TYPE JavaDoc distinguishing Glue uppercase from filter lowercase - Clarify deferred validation for default-table-format and table-type-filter --- catalogs/catalog-glue/build.gradle.kts | 1 + .../catalog/glue/GlueCatalogPropertiesMetadata.java | 12 ++++++++---- .../apache/gravitino/catalog/glue/GlueClientProvider.java | 10 +++++----- .../org/apache/gravitino/catalog/glue/GlueConstants.java | 9 +++++++-- .../catalog/glue/TestGlueCatalogPropertiesMetadata.java | 4 ++-- 5 files changed, 23 insertions(+), 13 deletions(-) diff --git a/catalogs/catalog-glue/build.gradle.kts b/catalogs/catalog-glue/build.gradle.kts index e28af8144f..b7787263c5 100644 --- a/catalogs/catalog-glue/build.gradle.kts +++ b/catalogs/catalog-glue/build.gradle.kts @@ -37,6 +37,7 @@ dependencies { implementation(libs.aws.glue) implementation(libs.aws.sts) + implementation(libs.commons.lang3) implementation(libs.guava) implementation(libs.slf4j.api) diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java index 443fc5a0c9..b2d631763e 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java @@ -49,10 +49,12 @@ public class GlueCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata false /* hidden */)) .put( AWS_GLUE_CATALOG_ID, - stringRequiredPropertyEntry( + stringOptionalPropertyEntry( AWS_GLUE_CATALOG_ID, - "The 12-digit AWS account ID that owns the Glue catalog", + "The 12-digit AWS account ID that owns the Glue catalog." + + " When omitted, defaults to the caller's AWS account ID.", true /* immutable */, + null /* defaultValue */, false /* hidden */)) .put( AWS_ACCESS_KEY_ID, @@ -85,7 +87,8 @@ public class GlueCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata DEFAULT_TABLE_FORMAT, stringOptionalPropertyEntry( DEFAULT_TABLE_FORMAT, - "Default format for tables created via createTable(). Accepted: iceberg, hive.", + "Default format for tables created via createTable(). Accepted: iceberg, hive." + + " Unrecognised values are rejected at createTable() time.", false /* immutable */, DEFAULT_TABLE_FORMAT_VALUE, false /* hidden */)) @@ -94,7 +97,8 @@ public class GlueCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata stringOptionalPropertyEntry( TABLE_TYPE_FILTER, "Comma-separated table types exposed by listTables() and loadTable()." - + " Accepted: all, hive, iceberg, delta, parquet.", + + " Accepted: all, hive, iceberg, delta, parquet." + + " Unrecognised values are rejected at listTables() time.", false /* immutable */, TABLE_TYPE_FILTER_ALL, false /* hidden */)) diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java index df0d37b641..3cf3e36036 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java @@ -19,9 +19,9 @@ package org.apache.gravitino.catalog.glue; import com.google.common.base.Preconditions; -import com.google.common.base.Strings; import java.net.URI; import java.util.Map; +import org.apache.commons.lang3.StringUtils; import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; @@ -57,7 +57,7 @@ public final class GlueClientProvider { public static GlueClient buildClient(Map<String, String> config) { String region = config.get(GlueConstants.AWS_REGION); Preconditions.checkArgument( - !Strings.isNullOrEmpty(region), + StringUtils.isNotBlank(region), "Property '%s' is required to create a Glue client", GlueConstants.AWS_REGION); @@ -67,8 +67,8 @@ public final class GlueClientProvider { // Both keys must be provided together — a partial pair is always a misconfiguration. String accessKey = config.get(GlueConstants.AWS_ACCESS_KEY_ID); String secretKey = config.get(GlueConstants.AWS_SECRET_ACCESS_KEY); - boolean hasAccessKey = !Strings.isNullOrEmpty(accessKey); - boolean hasSecretKey = !Strings.isNullOrEmpty(secretKey); + boolean hasAccessKey = StringUtils.isNotBlank(accessKey); + boolean hasSecretKey = StringUtils.isNotBlank(secretKey); Preconditions.checkArgument( hasAccessKey == hasSecretKey, "Both '%s' and '%s' must be set together. " @@ -86,7 +86,7 @@ public final class GlueClientProvider { // Optional custom endpoint override for VPC endpoints or LocalStack testing. String endpoint = config.get(GlueConstants.AWS_GLUE_ENDPOINT); - if (!Strings.isNullOrEmpty(endpoint)) { + if (StringUtils.isNotBlank(endpoint)) { try { builder.endpointOverride(URI.create(endpoint)); } catch (IllegalArgumentException e) { diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java index 90c4d0831f..f3ef784222 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java @@ -28,7 +28,10 @@ public final class GlueConstants { /** AWS region for the Glue Data Catalog (required). */ public static final String AWS_REGION = "aws-region"; - /** Glue catalog ID — the 12-digit AWS account ID (required). */ + /** + * Glue catalog ID — the 12-digit AWS account ID (optional). When omitted, defaults to the + * caller's AWS account ID. + */ public static final String AWS_GLUE_CATALOG_ID = "aws-glue-catalog-id"; /** AWS access key ID for static credential authentication (optional, sensitive). */ @@ -68,7 +71,9 @@ public final class GlueConstants { /** * Glue table format type parameter key stored in {@code Table.parameters()}. Common values: - * {@code ICEBERG}, {@code HIVE}, {@code DELTA}, {@code PARQUET}. + * {@code ICEBERG}, {@code HIVE}, {@code DELTA}, {@code PARQUET} (uppercase, as stored by Glue). + * Note: these differ from the Gravitino-side filter values in {@link #TABLE_TYPE_FILTER}, which + * use lowercase (e.g. {@code iceberg}, {@code hive}). */ public static final String TABLE_FORMAT_TYPE = "table_format_type"; diff --git a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogPropertiesMetadata.java b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogPropertiesMetadata.java index e150c5a57c..0cd1754f33 100644 --- a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogPropertiesMetadata.java +++ b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogPropertiesMetadata.java @@ -49,8 +49,8 @@ class TestGlueCatalogPropertiesMetadata { } @Test - void testAwsGlueCatalogIdIsRequired() { - assertTrue(metadata.isRequiredProperty(AWS_GLUE_CATALOG_ID)); + void testAwsGlueCatalogIdIsOptional() { + assertFalse(metadata.isRequiredProperty(AWS_GLUE_CATALOG_ID)); } @Test
