Copilot commented on code in PR #10384:
URL: https://github.com/apache/gravitino/pull/10384#discussion_r2918218777
##########
catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/storage/PaimonOSSFileSystemConfig.java:
##########
@@ -63,6 +64,14 @@ public PaimonOSSFileSystemConfig(Map<String, String>
properties) {
.checkValue(StringUtils::isNotBlank,
ConfigConstants.NOT_BLANK_ERROR_MSG)
.create();
+ public static final ConfigEntry<String> PAIMON_OSS_IMPLEMENT_ENTRY =
+ new ConfigBuilder(OSS_IMPLEMENT)
+ .doc("The filesystem implement of the Aliyun oss")
+ .version(ConfigConstants.VERSION_1_2_0)
+ .stringConf()
+ .checkValue(StringUtils::isNotBlank,
ConfigConstants.NOT_BLANK_ERROR_MSG)
+ .create();
Review Comment:
The new OSS implementation property is described as "filesystem implement"
in the ConfigEntry docs/metadata. Please rename this wording to "filesystem
implementation" (and consider capitalizing "OSS") so it reads correctly in
generated docs/UI.
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonPropertiesUtils.java:
##########
@@ -60,6 +60,8 @@ public class PaimonPropertiesUtils {
OSSProperties.GRAVITINO_OSS_ACCESS_KEY_ID,
PaimonConstants.OSS_ACCESS_KEY);
gravitinoConfigToPaimon.put(
OSSProperties.GRAVITINO_OSS_ACCESS_KEY_SECRET,
PaimonConstants.OSS_SECRET_KEY);
+ gravitinoConfigToPaimon.put(
+ OSSProperties.GRAVITINO_OSS_IMPLEMENT, PaimonConstants.OSS_IMPLEMENT);
Review Comment:
The new OSS implementation mapping is not covered by any of the existing
connector/unit tests (e.g., Spark/Flink Paimon properties converter tests only
cover backend/warehouse/uri). Please add/extend tests to assert that the
Gravitino key maps through to the expected Paimon/Spark/Flink config key, since
this PR’s core behavior is purely configuration translation.
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/storage/OSSProperties.java:
##########
@@ -29,6 +29,8 @@ public class OSSProperties {
public static final String GRAVITINO_OSS_ACCESS_KEY_ID = "oss-access-key-id";
// The static access key secret used to access OSS data.
public static final String GRAVITINO_OSS_ACCESS_KEY_SECRET =
"oss-secret-access-key";
+ // The filesystem implement of Aliyun OSS.
Review Comment:
Typo/grammar in the new constant comment: "filesystem implement" should be
"filesystem implementation".
```suggestion
// The filesystem implementation of Aliyun OSS.
```
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonConstants.java:
##########
@@ -48,6 +48,7 @@ public class PaimonConstants {
public static final String OSS_ENDPOINT = "fs.oss.endpoint";
public static final String OSS_ACCESS_KEY = "fs.oss.accessKeyId";
public static final String OSS_SECRET_KEY = "fs.oss.accessKeySecret";
+ public static final String OSS_IMPLEMENT = "hadoop.fs.oss.impl";
Review Comment:
PaimonConstants.OSS_IMPLEMENT is set to "hadoop.fs.oss.impl", while the
other OSS keys in this class use the unprefixed Hadoop key form ("fs.oss.*").
If this is intended as a Spark/Flink Hadoop-conf passthrough key, please add an
in-code comment explaining why the "hadoop." prefix is required here;
otherwise, consider using "fs.oss.impl" for consistency with the other OSS
constants and the issue description.
```suggestion
public static final String OSS_IMPLEMENT = "fs.oss.impl";
```
##########
catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/storage/PaimonOSSFileSystemConfig.java:
##########
@@ -101,5 +114,13 @@ public String getOSSSecretKey() {
false /* immutable */,
null /* defaultValue */,
false /* hidden */))
+ .put(
+ OSS_IMPLEMENT,
+ PropertyEntry.stringOptionalPropertyEntry(
+ OSS_IMPLEMENT,
+ "The filesystem implement of the Aliyun oss",
Review Comment:
The property metadata description repeats the same grammar issue
("filesystem implement"). Please update it to "filesystem implementation" for
clarity/consistency with other property descriptions.
```suggestion
"The filesystem implementation of the Aliyun oss",
```
##########
catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonOSSIT.java:
##########
@@ -60,6 +61,7 @@ protected Map<String, String> initPaimonCatalogProperties() {
catalogProperties.put(OSSProperties.GRAVITINO_OSS_ACCESS_KEY_ID,
accessKey);
catalogProperties.put(OSSProperties.GRAVITINO_OSS_ACCESS_KEY_SECRET,
secretKey);
catalogProperties.put(OSSProperties.GRAVITINO_OSS_ENDPOINT, endpoint);
+ catalogProperties.put(OSSProperties.GRAVITINO_OSS_IMPLEMENT, ossImpl);
Review Comment:
This IT now sets the new `oss-impl` property, but it doesn’t validate that
the value is actually propagated to the underlying Paimon/Spark configuration
(and `initSparkEnv` doesn’t set the corresponding OSS impl config key either).
To make this change testable, add an assertion on the transformed inner
properties (or configure Spark/Paimon with the impl key and verify it is
visible in the catalog’s Hadoop configuration).
--
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]
