Copilot commented on code in PR #11153:
URL: https://github.com/apache/gravitino/pull/11153#discussion_r3268207546
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/provider/DynamicIcebergConfigProvider.java:
##########
@@ -70,6 +71,7 @@ public void initialize(Map<String, String> properties) {
Optional.ofNullable(
properties.get(IcebergConstants.ICEBERG_REST_DEFAULT_DYNAMIC_CATALOG_NAME));
this.properties = properties;
+ this.serverConfigs =
StaticIcebergConfigProvider.initCatalogConfigs(properties);
}
Review Comment:
When `serverCatalogKey` is the default catalog (built from the full
`properties` map), `serverConfig.getAllConfig()` can include `catalog.<name>.*`
prefixed entries. Merging those into the returned `IcebergConfig` may
unintentionally introduce irrelevant/incorrect keys into the runtime catalog
configuration. Consider filtering server-side defaults before merging (e.g.,
exclude keys starting with `catalog.` for the default config), or only merging
known/allowed Iceberg REST keys to avoid leaking catalog-prefixed settings into
the final config map.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/provider/DynamicIcebergConfigProvider.java:
##########
@@ -100,7 +102,25 @@ public Optional<IcebergConfig>
getIcebergCatalogConfig(String catalogName) {
"lakehouse-iceberg".equals(catalog.provider()),
String.format("%s.%s is not iceberg catalog", gravitinoMetalake,
catalogName));
- return
Optional.of(getIcebergConfigFromCatalogProperties(catalog.properties()));
+ return Optional.of(mergeServerConfig(catalog.properties(), catalogName));
+ }
+
+ /**
+ * Merges Gravitino catalog properties with server-side defaults for the
given catalog key.
+ *
+ * <p>Catalog properties take precedence; missing keys are filled from the
server config entry
+ * identified by {@code serverCatalogKey} (for example {@link
+ * IcebergConstants#ICEBERG_REST_DEFAULT_CATALOG}).
+ */
+ private IcebergConfig mergeServerConfig(
+ Map<String, String> catalogProperties, String serverCatalogKey) {
+ Map<String, String> catalogConfig =
+ new
HashMap<>(getIcebergConfigFromCatalogProperties(catalogProperties).getAllConfig());
+ IcebergConfig serverConfig = serverConfigs.get(serverCatalogKey);
+ if (serverConfig != null) {
+ serverConfig.getAllConfig().forEach(catalogConfig::putIfAbsent);
+ }
+ return new IcebergConfig(catalogConfig);
}
Review Comment:
When `serverCatalogKey` is the default catalog (built from the full
`properties` map), `serverConfig.getAllConfig()` can include `catalog.<name>.*`
prefixed entries. Merging those into the returned `IcebergConfig` may
unintentionally introduce irrelevant/incorrect keys into the runtime catalog
configuration. Consider filtering server-side defaults before merging (e.g.,
exclude keys starting with `catalog.` for the default config), or only merging
known/allowed Iceberg REST keys to avoid leaking catalog-prefixed settings into
the final config map.
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/IcebergConfig.java:
##########
@@ -283,7 +283,7 @@ public class IcebergConfig extends Config implements
OverwriteDefaultConfig {
.doc("Table metadata cache capacity")
.version(ConfigConstants.VERSION_1_1_0)
.intConf()
- .createWithDefault(200);
+ .createWithDefault(1000);
Review Comment:
Increasing the default table metadata cache capacity from 200 to 1000 can
significantly increase baseline memory usage, depending on entry size and
eviction behavior. If this applies broadly (not only to REST server), consider
documenting sizing guidance and/or ensuring the default is appropriate for
typical deployments (or making the new default scoped to the component that
needs it).
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/provider/TestDynamicIcebergConfigProvider.java:
##########
@@ -257,6 +257,97 @@ void testIcebergConfig() {
icebergConfig.getIcebergCatalogProperties().get("catalog.backend-name"),
"custom_backend");
}
+ @Test
+ void testLoadServerCatalogConfigs() {
+ Map<String, String> properties = new HashMap<>();
+ properties.put(IcebergConstants.GRAVITINO_METALAKE, "test_metalake");
+ properties.put(IcebergConstants.TABLE_METADATA_CACHE_IMPL,
"default-cache");
+ properties.put("catalog.jdbc_backend.catalog-backend", "jdbc");
+ properties.put("catalog.jdbc_backend.jdbc.user", "static-user");
+
+ Map<String, IcebergConfig> catalogConfigs =
+ StaticIcebergConfigProvider.initCatalogConfigs(properties);
+
+ Assertions.assertEquals(
+ "default-cache",
+ catalogConfigs
+ .get(IcebergConstants.ICEBERG_REST_DEFAULT_CATALOG)
+ .get(IcebergConfig.TABLE_METADATA_CACHE_IMPL));
+ Assertions.assertEquals(
+ "jdbc",
catalogConfigs.get("jdbc_backend").get(IcebergConfig.CATALOG_BACKEND));
+ Assertions.assertEquals(
+ "static-user",
catalogConfigs.get("jdbc_backend").getAllConfig().get("jdbc.user"));
+ }
+
+ @Test
+ void testNamedDynamicCatalogMergesNamedStaticConfigOnly() {
+ String metalakeName = "test_metalake";
+ String catalogName = "jdbc_backend";
+ Catalog mockCatalog = Mockito.mock(Catalog.class);
+ Mockito.when(mockCatalog.provider()).thenReturn("lakehouse-iceberg");
+ Mockito.when(mockCatalog.properties())
+ .thenReturn(
+ new HashMap<String, String>() {
+ {
+ put(IcebergConstants.CATALOG_BACKEND, "jdbc");
+ put(IcebergConstants.CATALOG_BACKEND_NAME, catalogName);
+ put(IcebergConstants.URI, "jdbc:sqlite::memory:");
+ }
+ });
Review Comment:
Double-brace initialization creates an anonymous class and can retain an
implicit reference to the enclosing instance, which is unnecessary here (even
in tests). Prefer a regular `HashMap` plus `put(...)` calls, or `Map.of(...)`
if immutability is acceptable.
--
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]
