yuqi1129 commented on code in PR #11713: URL: https://github.com/apache/gravitino/pull/11713#discussion_r3432767204
########## catalogs/catalog-common/src/main/java/org/apache/gravitino/credential/config/COSCredentialConfig.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.credential.config; + +import java.util.Map; +import javax.validation.constraints.NotNull; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.config.ConfigBuilder; +import org.apache.gravitino.config.ConfigConstants; +import org.apache.gravitino.config.ConfigEntry; +import org.apache.gravitino.storage.COSProperties; + +/** + * Slim credential config for Tencent Cloud COS, covering only the static secret-key path. STS / + * token-related entries (role arn, token expire) will be added by a follow-up PR that introduces + * the dynamic credential vending support. + */ +public class COSCredentialConfig extends Config { + + public static final ConfigEntry<String> COS_REGION = + new ConfigBuilder(COSProperties.GRAVITINO_COS_REGION) + .doc("The region of the Tencent Cloud COS service") + .version(ConfigConstants.VERSION_1_3_0) + .stringConf() + .create(); + + public static final ConfigEntry<String> COS_ACCESS_KEY_ID = + new ConfigBuilder(COSProperties.GRAVITINO_COS_ACCESS_KEY_ID) + .doc("The static access key ID (Tencent Cloud SecretId) used to access COS data") + .version(ConfigConstants.VERSION_1_3_0) + .stringConf() + .checkValue(StringUtils::isNotBlank, ConfigConstants.NOT_BLANK_ERROR_MSG) + .create(); + + public static final ConfigEntry<String> COS_SECRET_ACCESS_KEY = + new ConfigBuilder(COSProperties.GRAVITINO_COS_ACCESS_KEY_SECRET) + .doc("The static secret access key (Tencent Cloud SecretKey) used to access COS data") + .version(ConfigConstants.VERSION_1_3_0) + .stringConf() + .checkValue(StringUtils::isNotBlank, ConfigConstants.NOT_BLANK_ERROR_MSG) + .create(); + + public COSCredentialConfig(Map<String, String> properties) { + super(false); + loadFromMap(properties, k -> true); + } + + public String region() { Review Comment: `region()` isn't used anywhere in this PR (the provider only reads AK/SK), and `COS_REGION` has no `checkValue`/default. Fine as forward-looking code for the STS PR, just noting it's currently unused. ########## bundles/tencent/src/main/java/org/apache/gravitino/cos/fs/COSCredentialsProvider.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.cos.fs; + +import com.qcloud.cos.auth.BasicCOSCredentials; +import com.qcloud.cos.auth.COSCredentials; +import java.net.URI; +import org.apache.gravitino.catalog.hadoop.fs.FileSystemUtils; +import org.apache.gravitino.catalog.hadoop.fs.GravitinoFileSystemCredentialsProvider; +import org.apache.gravitino.credential.COSSecretKeyCredential; +import org.apache.gravitino.credential.Credential; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.auth.AbstractCOSCredentialProvider; + +/** + * Hadoop-COS credential provider that pulls vended credentials out of Gravitino and feeds them to + * the underlying {@code com.qcloud.cos.auth.COSCredentialsProvider} contract. PR-A only handles + * static secret-key credentials; STS / token credentials will be added by a follow-up PR. + */ +public class COSCredentialsProvider extends AbstractCOSCredentialProvider { + + private final GravitinoFileSystemCredentialsProvider gravitinoFileSystemCredentialsProvider; + private volatile COSCredentials basicCredentials; + private volatile long expirationTime = Long.MAX_VALUE; + private static final double EXPIRATION_TIME_FACTOR = 0.5D; + + public COSCredentialsProvider(URI uri, Configuration conf) { + super(uri, conf); + this.gravitinoFileSystemCredentialsProvider = FileSystemUtils.getGvfsCredentialProvider(conf); + } + + @Override + public COSCredentials getCredentials() { + if (basicCredentials == null || System.currentTimeMillis() >= expirationTime) { + synchronized (this) { + if (basicCredentials == null || System.currentTimeMillis() >= expirationTime) { + refresh(); + } + } + } + return basicCredentials; + } + + @Override + public void refresh() { + Credential[] gravitinoCredentials = gravitinoFileSystemCredentialsProvider.getCredentials(); + Credential credential = COSUtils.getSuitableCredential(gravitinoCredentials); + if (credential == null) { + throw new RuntimeException("No suitable credential for COS found..."); + } + + if (credential instanceof COSSecretKeyCredential) { Review Comment: If `getSuitableCredential` ever returns a credential that is not a `COSSecretKeyCredential`, `basicCredentials` stays `null` and no error is raised — `getCredentials()` then returns `null`, which will NPE deep inside the COS SDK. Safe today (the helper only returns secret-key or null), but once STS/token credentials are added in PR-B this becomes a trap. Consider an `else { throw ... }`. ########## catalogs/catalog-common/src/main/java/org/apache/gravitino/credential/config/COSCredentialConfig.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.credential.config; + +import java.util.Map; +import javax.validation.constraints.NotNull; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.config.ConfigBuilder; +import org.apache.gravitino.config.ConfigConstants; +import org.apache.gravitino.config.ConfigEntry; +import org.apache.gravitino.storage.COSProperties; + +/** + * Slim credential config for Tencent Cloud COS, covering only the static secret-key path. STS / + * token-related entries (role arn, token expire) will be added by a follow-up PR that introduces + * the dynamic credential vending support. + */ +public class COSCredentialConfig extends Config { + + public static final ConfigEntry<String> COS_REGION = + new ConfigBuilder(COSProperties.GRAVITINO_COS_REGION) + .doc("The region of the Tencent Cloud COS service") + .version(ConfigConstants.VERSION_1_3_0) Review Comment: Version markers say `1.3.0`, but `main` is currently `1.4.0-SNAPSHOT` and `ConfigConstants` has no `VERSION_1_4_0` (the latest is `VERSION_1_3_0`, and `CURRENT_SCRIPT_VERSION = VERSION_1_3_0`). Could you confirm the target release? If this lands in 1.4.0, these `.version(...)` markers (lines 41/48/56) and all the "Since version 1.3.0" entries in the docs should be bumped to 1.4.0, with a `VERSION_1_4_0` constant added. ########## bundles/tencent/src/main/java/org/apache/gravitino/cos/credential/COSSecretKeyProvider.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.cos.credential; + +import java.util.Map; +import org.apache.gravitino.credential.COSSecretKeyCredential; +import org.apache.gravitino.credential.Credential; +import org.apache.gravitino.credential.CredentialContext; +import org.apache.gravitino.credential.CredentialProvider; +import org.apache.gravitino.credential.config.COSCredentialConfig; + +/** Generate COS access key and secret key to access Tencent Cloud COS data. */ +public class COSSecretKeyProvider implements CredentialProvider { + + private String accessKey; + private String secretKey; + + @Override + public void initialize(Map<String, String> properties) { + COSCredentialConfig cosCredentialConfig = new COSCredentialConfig(properties); + this.accessKey = cosCredentialConfig.accessKeyID(); + this.secretKey = cosCredentialConfig.secretAccessKey(); + } + + @Override + public void close() {} + + @Override + public boolean supportsScheme(String scheme) { + // hadoop-cos exposes the `cosn://` scheme; we accept both spellings to be lenient. + return "cosn".equalsIgnoreCase(scheme) || "cos".equalsIgnoreCase(scheme); Review Comment: Minor: `supportsScheme` accepts both `cos` and `cosn`, but `COSFileSystemProvider.scheme()` only advertises `cosn`. Harmless, just flagging the inconsistency (the comment already explains the leniency). ########## catalogs/hadoop-common/src/main/java/org/apache/gravitino/catalog/hadoop/fs/Constants.java: ########## @@ -67,6 +67,10 @@ public class Constants { public static final String OSS_ESTABLISH_TIMEOUT_KEY = "fs.oss.connection.establish.timeout"; public static final String OSS_MAX_ERROR_RETRIES_KEY = "fs.oss.attempts.maximum"; + // Tencent Cloud COS (hadoop-cos / CosNFileSystem) specific configuration keys + public static final String COS_CONNECTION_TIMEOUT_KEY = "fs.cosn.connection.timeout"; Review Comment: `fs.cosn.maxRetries` on the next line is a real hadoop-cos key (`CosNConfigKeys.COSN_MAX_RETRIES_KEY`), but I can't find `fs.cosn.connection.timeout` anywhere in `CosNConfigKeys` — hadoop-cos doesn't appear to expose a connection-timeout key at all. If the name is wrong, the default injected in `COSFileSystemProvider.additionalCOSConfig` is silently ignored. Please verify against hadoop-cos `3.3.0-8.3.23`, and prefer referencing `CosNConfigKeys.*` constants (as the credential key mapping already does) over hardcoded literals. ########## bundles/tencent/src/main/java/org/apache/gravitino/cos/fs/COSCredentialsProvider.java: ########## @@ -0,0 +1,84 @@ +/* + * 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.cos.fs; + +import com.qcloud.cos.auth.BasicCOSCredentials; +import com.qcloud.cos.auth.COSCredentials; +import java.net.URI; +import org.apache.gravitino.catalog.hadoop.fs.FileSystemUtils; +import org.apache.gravitino.catalog.hadoop.fs.GravitinoFileSystemCredentialsProvider; +import org.apache.gravitino.credential.COSSecretKeyCredential; +import org.apache.gravitino.credential.Credential; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.auth.AbstractCOSCredentialProvider; + +/** + * Hadoop-COS credential provider that pulls vended credentials out of Gravitino and feeds them to + * the underlying {@code com.qcloud.cos.auth.COSCredentialsProvider} contract. PR-A only handles + * static secret-key credentials; STS / token credentials will be added by a follow-up PR. + */ +public class COSCredentialsProvider extends AbstractCOSCredentialProvider { Review Comment: This Hadoop-side bridge (the `refresh()`/`getCredentials()` caching + expiration logic) has no unit test, while the other COS classes do. Worth adding a small test that feeds a `COSSecretKeyCredential` through a stub `GravitinoFileSystemCredentialsProvider` and asserts the resulting `BasicCOSCredentials`. ########## docs/fileset-catalog-with-cos.md: ########## @@ -0,0 +1,548 @@ +--- +title: "Fileset Catalog with COS" +slug: "/fileset-catalog-with-cos" +date: 2026-06-17 +keyword: "Fileset catalog COS Tencent" +license: "This software is licensed under the Apache License version 2." +--- + +## Introduction + +This document explains how to configure a Fileset catalog with Tencent Cloud COS (Cloud Object Storage) in Gravitino. + +## Prerequisites + +To set up a Fileset catalog with COS, follow these steps: + +1. Download the [`gravitino-tencent-bundle-${gravitino-version}.jar`](https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-tencent-bundle) file. +2. Place the downloaded file into the Gravitino Fileset catalog classpath at `${GRAVITINO_HOME}/catalogs/fileset/libs/`. +3. Start the Gravitino server by running the following command: + +```bash +$ ${GRAVITINO_HOME}/bin/gravitino-server.sh start +``` + +Once the server is up and running, you can proceed to configure the Fileset catalog with COS. In the rest of this document we will use `http://localhost:8090` as the Gravitino server URL, replace with your actual server URL. + +## COS Catalog Configuration + +### COS Fileset Catalog Configuration + +In addition to the basic configurations mentioned in [Fileset-catalog-catalog-configuration](./fileset-catalog.md#catalog-properties), the following properties are required to configure a Fileset catalog with COS: + +| Configuration item | Description | Default value | Required | Since version | +|-------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------|---------------| +| `cos-region` | The region of the Tencent Cloud COS bucket, e.g. `ap-guangzhou`, `ap-shanghai`. | (none) | Yes | 1.3.0 | +| `cos-endpoint` | The endpoint of the Tencent Cloud COS service. Optional; when not set, hadoop-cos derives it from `cos-region` (`cos.${region}.myqcloud.com`). Set this only if you need to point to a non-public endpoint (e.g. an internal/VPC endpoint). | (none) | No | 1.3.0 | Review Comment: `cos-endpoint` actually maps to `fs.cosn.bucket.endpoint_suffix`, i.e. it is an endpoint *suffix* (e.g. `cos.ap-guangzhou.myqcloud.com`), not a full URL. Worth clarifying here so users don't pass `https://...`. -- 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]
