lasdf1234 commented on code in PR #13354: URL: https://github.com/apache/gravitino/pull/13354#discussion_r4060425557
########## core/src/main/java/org/apache/gravitino/cloud/storage/AWSPropertiesMetadata.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.cloud.storage; + +import static org.apache.gravitino.connector.PropertyEntry.stringOptionalPropertyEntry; + +import com.google.common.collect.ImmutableMap; +import java.util.Map; +import org.apache.gravitino.catalog.glue.GlueConstants; Review Comment: Done. The two names now live in org.apache.gravitino.storage.AWSProperties, next to S3Properties. GlueConstants.AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY delegate to those constants, and AWSPropertiesMetadata reads them from there instead of GlueConstants. aws-region, aws-glue-catalog-id, and aws-glue-endpoint stay in GlueConstants. They are Glue-only and are not part of the map merged into every catalog. ########## core/src/main/java/org/apache/gravitino/connector/BaseCatalogPropertiesMetadata.java: ########## @@ -102,17 +108,37 @@ protected Map<String, PropertyEntry<?>> specificPropertyEntries() { true /* hidden */)), PropertyEntry::getName); + /** + * Cloud credential keys merged into every catalog. A catalog that already declares a key wins. + */ + private static final Map<String, PropertyEntry<?>> CLOUD_PROPERTY_ENTRIES = + ImmutableMap.<String, PropertyEntry<?>>builder() + .putAll(S3PropertiesMetadata.PROPERTY_ENTRIES) + .putAll(OSSPropertiesMetadata.PROPERTY_ENTRIES) + .putAll(AzurePropertiesMetadata.PROPERTY_ENTRIES) + .putAll(GCSPropertiesMetadata.PROPERTY_ENTRIES) + .putAll(COSPropertiesMetadata.PROPERTY_ENTRIES) + .putAll(AWSPropertiesMetadata.PROPERTY_ENTRIES) + .build(); Review Comment: Done. The combined maps now live in CloudPropertiesMetadata. ALL_PROPERTY_ENTRIES (S3, OSS, Azure, GCS, COS, and the AWS access-key pair) is what BaseCatalogPropertiesMetadata and FallbackPropertiesMetadata use. Hive, Iceberg, Paimon, and Fileset catalog metadata no longer repeat that putAll; they extend BaseCatalogPropertiesMetadata, and a key the catalog already declares still wins. Schema and fileset metadata do not extend that base class, so they reference STORAGE_PROPERTY_ENTRIES instead. That set is the five storage maps only, without the AWS pair. aws-access-key-id stays a Glue catalog property and is not declared on a fileset or schema. ########## core/src/main/java/org/apache/gravitino/cloud/storage/AWSPropertiesMetadata.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.cloud.storage; + +import static org.apache.gravitino.connector.PropertyEntry.stringOptionalPropertyEntry; + +import com.google.common.collect.ImmutableMap; +import java.util.Map; +import org.apache.gravitino.catalog.glue.GlueConstants; +import org.apache.gravitino.connector.PropertyEntry; + +/** Shared AWS credential {@link PropertyEntry} definitions for catalog properties metadata. */ +public final class AWSPropertiesMetadata { + + /** AWS access key ID. Not hidden. */ + public static final PropertyEntry<String> AWS_ACCESS_KEY_ID = + stringOptionalPropertyEntry( + GlueConstants.AWS_ACCESS_KEY_ID, + "AWS access key ID for static credential authentication." + + " When omitted the default credential chain is used.", + false /* immutable */, + null /* defaultValue */, + false /* hidden */); + Review Comment: Thank you very much for your review. The file "design-docs/gravitino-glue-catalog.md" has been modified. The "aws-access-key-id" has been made non-protected for readers, and the interface returns the plaintext. ########## core/src/main/java/org/apache/gravitino/secret/FallbackPropertiesMetadata.java: ########## @@ -48,6 +49,7 @@ final class FallbackPropertiesMetadata extends BasePropertiesMetadata { .putAll(AzurePropertiesMetadata.PROPERTY_ENTRIES) .putAll(GCSPropertiesMetadata.PROPERTY_ENTRIES) .putAll(COSPropertiesMetadata.PROPERTY_ENTRIES) + .putAll(AWSPropertiesMetadata.PROPERTY_ENTRIES) Review Comment: Intentional. The aws-* to s3-* copy happens only in GlueCatalog.propertiesWithCredentialProviders(), on the catalog. Schema and fileset metadata already include S3/OSS/Azure/GCS/COS because a fileset location can carry those storage keys. aws-access-key-id is a Glue catalog property, not a fileset or schema property, so those two classes should not declare it. -- 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]
