Copilot commented on code in PR #10403:
URL: https://github.com/apache/gravitino/pull/10403#discussion_r2923767332
##########
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_FILESYSTEM_IMPLEMENTATION =
"hadoop.fs.oss.impl";
Review Comment:
`OSS_FILESYSTEM_IMPLEMENTATION` is set to `hadoop.fs.oss.impl`, but the OSS
implementation key used elsewhere in the repo (and in Hadoop config) is
`fs.oss.impl` (see `bundles/aliyun/.../OSSFileSystemProvider`). With the
current value, the new `oss-filesystem-impl` property will be pushed down to a
key Paimon/Hadoop likely won’t read, so the feature won’t work.
Suggestion: change this constant to `fs.oss.impl` (and keep the naming
consistent with the other OSS keys in this class, which are all `fs.oss.*`).
```suggestion
public static final String OSS_FILESYSTEM_IMPLEMENTATION = "fs.oss.impl";
```
##########
catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/integration/test/CatalogPaimonOSSIT.java:
##########
@@ -41,6 +41,7 @@ public class CatalogPaimonOSSIT extends CatalogPaimonBaseIT {
private static final String secretKey = "YOUR_SECRET_KEY";
private static final String endpoint = "OSS_ENDPOINT";
private static final String warehouse = "oss://" + OSS_BUCKET_NAME +
"/paimon-test";
+ private static final String ossImpl = "OSS_IMPL";
Review Comment:
`ossImpl` is currently a placeholder value (`"OSS_IMPL"`), which makes it
unclear what concrete value users should provide for `oss-filesystem-impl` (it
should be a fully-qualified filesystem implementation class name). This also
means that if someone enables this IT locally, it will still fail unless they
infer the right class name.
Suggestion: set the example value to the expected implementation class
(e.g., Aliyun OSS FS implementation) and (if required for the Spark side)
propagate it into `initSparkEnv()` as the corresponding Spark catalog option
key.
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonPropertiesUtils.java:
##########
@@ -60,6 +60,9 @@ 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_FILESYSTEM_IMPLEMENTATION,
+ PaimonConstants.OSS_FILESYSTEM_IMPLEMENTATION);
GRAVITINO_CONFIG_TO_PAIMON =
Collections.unmodifiableMap(gravitinoConfigToPaimon);
Review Comment:
This new behavior (mapping `oss-filesystem-impl` into Paimon catalog
properties) doesn’t appear to be covered by any non-disabled automated test.
The only updated OSS test is `@Disabled`, so CI won’t validate that the new
property is actually propagated into Spark/Flink/Paimon options.
Suggestion: add/extend a unit test that exercises the conversion path (e.g.,
via the existing Spark `TestPaimonPropertiesConverter`, or a dedicated test for
`PaimonPropertiesUtils.toPaimonCatalogProperties`) and assert the new key is
present in the converted map.
--
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]
