Copilot commented on code in PR #11694:
URL: https://github.com/apache/gravitino/pull/11694#discussion_r3426230129
##########
flink-connector/flink-common/src/main/java/org/apache/gravitino/flink/connector/iceberg/GravitinoIcebergCatalogFactory.java:
##########
@@ -122,35 +122,62 @@ Map<String, String> toIcebergCatalogOptions(Map<String,
String> catalogOptions)
Map<String, String> icebergCatalogOptions =
Maps.newHashMap(catalogOptions);
String catalogBackend =
catalogOptions.get(IcebergPropertiesConstants.GRAVITINO_ICEBERG_CATALOG_BACKEND);
- // Only infer `catalog-type` from the backend when neither `catalog-type`
nor `catalog-impl` is
- // already set, otherwise an explicitly provided `catalog-impl` would
conflict with it.
+ // catalogBackend is only present here on the CREATE CATALOG path (raw
user SQL options). On
+ // the USE CATALOG path (loading a catalog already persisted in Gravitino),
+ // GravitinoCatalogStore.getCatalog() has already renamed catalog-backend
-> catalog-type
+ // before this method runs, so catalogBackend is null even for a REST/JDBC
catalog. This
+ // normalizes the CREATE CATALOG case so both paths converge on the same
catalog-type key.
Review Comment:
The comment attributes the `catalog-backend` -> `catalog-type` rename to
`GravitinoCatalogStore.getCatalog()`, but the actual mapping is performed by
`IcebergPropertiesConverter` during property conversion. This is a
maintainability/documentation issue because future refactors may look in the
wrong place for the behavior.
##########
flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/iceberg/TestGravitinoIcebergCatalogFactory.java:
##########
@@ -115,4 +119,27 @@ void testRestBackendKeepsCatalogType() {
Assertions.assertFalse(result.containsKey(IcebergPropertiesConstants.ICEBERG_CATALOG_IMPL));
Assertions.assertEquals("iceberg",
result.get(CommonCatalogOptions.CATALOG_TYPE.key()));
}
+
+ @Test
+ void testRestAuthPropagatedWhenLoadedFromCatalogStore() {
+ // Simulates the catalog-store load path (`USE CATALOG ...`): by the time
options reach this
+ // factory, GravitinoCatalogStore.getCatalog has already renamed
`catalog-backend` ->
+ // `catalog-type`, so only `catalog-type=rest` is present, no
`catalog-backend` key.
Review Comment:
This comment says `GravitinoCatalogStore.getCatalog` renamed
`catalog-backend` -> `catalog-type`, but that conversion happens in
`IcebergPropertiesConverter` when translating Gravitino catalog properties into
Flink/Iceberg options. Tightening the wording will make the test’s intent
clearer.
##########
flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/iceberg/TestGravitinoIcebergCatalogFactory.java:
##########
@@ -105,7 +105,11 @@ void testRestBackendKeepsCatalogType() {
IcebergPropertiesConstants.ICEBERG_CATALOG_TYPE,
IcebergPropertiesConstants.ICEBERG_CATALOG_BACKEND_REST,
IcebergPropertiesConstants.ICEBERG_CATALOG_URI,
- "http://localhost:9001/iceberg/";);
+ "http://localhost:9001/iceberg/";,
+ // Pre-set so the REST auth-propagation branch (which needs a live
+ // GravitinoCatalogManager) is skipped; that branch is covered
separately below.
+ org.apache.iceberg.rest.auth.AuthProperties.AUTH_TYPE,
+ "none");
Review Comment:
Avoid using a fully qualified class name for `AuthProperties` here; the repo
conventions prefer normal imports unless there is a name conflict
(AGENTS.md:26-31). Import `org.apache.iceberg.rest.auth.AuthProperties` and
reference `AuthProperties.AUTH_TYPE` (and optionally
`AuthProperties.AUTH_TYPE_NONE`) directly.
--
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]
