Copilot commented on code in PR #10726: URL: https://github.com/apache/gravitino/pull/10726#discussion_r3056984934
########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java: ########## @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +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 software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.GlueClientBuilder; + +/** + * Factory for creating AWS {@link GlueClient} instances from Gravitino catalog configuration. + * + * <p>Authentication priority: + * + * <ol> + * <li>Static credentials ({@code aws-access-key-id} + {@code aws-secret-access-key}) + * <li>Default credential chain (environment variables, instance profile, container credentials) + * </ol> + * + * <p>An optional endpoint override ({@code aws-glue-endpoint}) enables connectivity to VPC + * endpoints and LocalStack for integration testing. + */ +public final class GlueClientProvider { + + private GlueClientProvider() {} + + /** + * Builds a {@link GlueClient} from the given catalog configuration map. + * + * @param config Catalog configuration properties. + * @return A configured and ready-to-use {@link GlueClient}. + * @throws IllegalArgumentException if {@code aws-region} is missing or blank, if only one of the + * credential keys is provided, or if {@code aws-glue-endpoint} is not a valid URI. + */ + public static GlueClient buildClient(Map<String, String> config) { + String region = config.get(GlueConstants.AWS_REGION); + Preconditions.checkArgument( + !Strings.isNullOrEmpty(region), + "Property '%s' is required to create a Glue client", + GlueConstants.AWS_REGION); + + GlueClientBuilder builder = GlueClient.builder().region(Region.of(region)); + + // Static credentials take priority over the default credential chain. + // 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); + Preconditions.checkArgument( + hasAccessKey == hasSecretKey, + "Both '%s' and '%s' must be set together. " + + "Either provide both keys for static authentication, " + + "or omit both to use the default credential chain.", + GlueConstants.AWS_ACCESS_KEY_ID, + GlueConstants.AWS_SECRET_ACCESS_KEY); + + if (hasAccessKey) { + builder.credentialsProvider( + StaticCredentialsProvider.create(AwsBasicCredentials.create(accessKey, secretKey))); + } else { + builder.credentialsProvider(DefaultCredentialsProvider.create()); + } Review Comment: Whitespace-only credential values (e.g., `" "`) are currently treated as “present”, so the code can build `AwsBasicCredentials` with effectively blank keys and proceed, resulting in harder-to-diagnose authentication failures later. Consider treating whitespace-only values as absent (same approach as region) before computing `hasAccessKey`/`hasSecretKey`, so misconfigurations fail fast with the existing `hasAccessKey == hasSecretKey` check (or with an explicit “must not be blank” message). ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueClientProvider.java: ########## @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +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 software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.GlueClientBuilder; + +/** + * Factory for creating AWS {@link GlueClient} instances from Gravitino catalog configuration. + * + * <p>Authentication priority: + * + * <ol> + * <li>Static credentials ({@code aws-access-key-id} + {@code aws-secret-access-key}) + * <li>Default credential chain (environment variables, instance profile, container credentials) + * </ol> + * + * <p>An optional endpoint override ({@code aws-glue-endpoint}) enables connectivity to VPC + * endpoints and LocalStack for integration testing. + */ +public final class GlueClientProvider { + + private GlueClientProvider() {} + + /** + * Builds a {@link GlueClient} from the given catalog configuration map. + * + * @param config Catalog configuration properties. + * @return A configured and ready-to-use {@link GlueClient}. + * @throws IllegalArgumentException if {@code aws-region} is missing or blank, if only one of the + * credential keys is provided, or if {@code aws-glue-endpoint} is not a valid URI. + */ + public static GlueClient buildClient(Map<String, String> config) { + String region = config.get(GlueConstants.AWS_REGION); + Preconditions.checkArgument( + !Strings.isNullOrEmpty(region), + "Property '%s' is required to create a Glue client", + GlueConstants.AWS_REGION); + + GlueClientBuilder builder = GlueClient.builder().region(Region.of(region)); Review Comment: `Strings.isNullOrEmpty(region)` does not reject whitespace-only values (e.g., `" "`), so a blank region will pass the precondition and then fail later (or unexpectedly) when building the `Region`. This also makes `TestGlueClientProvider#testBuildClientBlankRegionThrows` fail. Treat blank/whitespace-only region values as invalid by checking `isBlank()` (or `trim().isEmpty()`) in the precondition. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTablePropertiesMetadata.java: ########## @@ -26,12 +30,36 @@ /** * Properties metadata for Glue tables. * - * <p>TODO PR-02: support passthrough of Glue Table.parameters() keys such as {@code table_type} and - * {@code metadata_location} for Iceberg, Delta, and other formats. + * <p>Defines well-known Glue {@code Table.parameters()} keys that Gravitino exposes. All entries + * are optional and mutable, reflecting that Glue stores them as free-form key-value pairs. Unknown + * parameters from {@code Table.parameters()} are passed through transparently by the catalog + * operations layer and are not validated here. + * + * <p>Note: storage location ({@code StorageDescriptor.location}) varies by table format and is + * handled per-format in the Table CRUD layer, not declared here. */ public class GlueTablePropertiesMetadata extends BasePropertiesMetadata { - private static final Map<String, PropertyEntry<?>> PROPERTIES_METADATA = ImmutableMap.of(); + private static final Map<String, PropertyEntry<?>> PROPERTIES_METADATA = + ImmutableMap.<String, PropertyEntry<?>>builder() + .put( + TABLE_FORMAT_TYPE, + stringOptionalPropertyEntry( + TABLE_FORMAT_TYPE, + "Glue table format type stored in Table.parameters(). Common values:" + + " ICEBERG, HIVE, DELTA, PARQUET.", + false /* immutable */, + null /* defaultValue */, + false /* hidden */)) + .put( + METADATA_LOCATION, + stringOptionalPropertyEntry( + METADATA_LOCATION, + "Iceberg metadata file location stored in Table.parameters().", + false /* immutable */, + null /* defaultValue */, + false /* hidden */)) + .build(); Review Comment: This PR introduces concrete, user-visible table property metadata entries (`table_format_type`, `metadata_location`) but does not add a unit test asserting they exist and are optional/mutable/not hidden. Adding a focused test (similar to `TestGlueCatalogPropertiesMetadata`) would help prevent regressions in property keys, defaults, or visibility flags. -- 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]
