Copilot commented on code in PR #11156:
URL: https://github.com/apache/gravitino/pull/11156#discussion_r3268273871
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/IcebergConfig.java:
##########
@@ -276,14 +277,14 @@ public class IcebergConfig extends Config implements
OverwriteDefaultConfig {
.doc("Table metadata cache implementation")
.version(ConfigConstants.VERSION_1_1_0)
.stringConf()
- .create();
+ .createWithDefault(LocalTableMetadataCache.class.getName());
Review Comment:
Setting a default `TABLE_METADATA_CACHE_IMPL` (previously unset) and
increasing the default capacity changes runtime behavior on upgrade: the cache
will now be enabled by default and may consume more heap. If this behavior
change is intended, consider adding an explicit opt-in/enable flag or keeping
`TABLE_METADATA_CACHE_IMPL` default unset and only enabling when configured;
otherwise, document this as a breaking behavioral change in release/migration
notes.
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/IcebergConfig.java:
##########
@@ -276,14 +277,14 @@ public class IcebergConfig extends Config implements
OverwriteDefaultConfig {
.doc("Table metadata cache implementation")
.version(ConfigConstants.VERSION_1_1_0)
.stringConf()
- .create();
+ .createWithDefault(LocalTableMetadataCache.class.getName());
public static final ConfigEntry<Integer> TABLE_METADATA_CACHE_CAPACITY =
new ConfigBuilder(IcebergConstants.TABLE_METADATA_CACHE_CAPACITY)
.doc("Table metadata cache capacity")
.version(ConfigConstants.VERSION_1_1_0)
.intConf()
- .createWithDefault(200);
+ .createWithDefault(1000);
Review Comment:
Setting a default `TABLE_METADATA_CACHE_IMPL` (previously unset) and
increasing the default capacity changes runtime behavior on upgrade: the cache
will now be enabled by default and may consume more heap. If this behavior
change is intended, consider adding an explicit opt-in/enable flag or keeping
`TABLE_METADATA_CACHE_IMPL` default unset and only enabling when configured;
otherwise, document this as a breaking behavioral change in release/migration
notes.
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -457,11 +457,13 @@ private TableMetadataCache
loadTableMetadataCache(IcebergConfig config, Catalog
return TableMetadataCache.DUMMY;
}
- Preconditions.checkArgument(
- catalog instanceof SupportsMetadataLocation,
- "You shouldn't enable Iceberg metadata cache for the catalog %s,"
- + " because the catalog impl does not support get metadata
location.",
- catalog.name());
+ if (!(catalog instanceof SupportsMetadataLocation)) {
+ LOG.warn(
+ "Skip Iceberg table metadata cache for catalog {} because the
catalog does not support"
+ + " metadata location.",
+ catalog.name());
+ return TableMetadataCache.DUMMY;
+ }
Review Comment:
This warning will likely trigger for any catalog that doesn’t implement
`SupportsMetadataLocation`. With the cache implementation now defaulted, this
can become noisy in normal/expected deployments. Consider lowering to
INFO/DEBUG or only warning when the user explicitly configured a non-default
cache impl (vs. default/implicit behavior).
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogPropertiesMetadata.java:
##########
@@ -127,13 +127,14 @@ public class IcebergCatalogPropertiesMetadata extends
BaseCatalogPropertiesMetad
IcebergConstants.TABLE_METADATA_CACHE_IMPL,
"Table metadata cache implementation",
false /* immutable */,
- null /* defaultValue */,
+
"org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache"
+ /* defaultValue */,
false /* hidden */),
integerOptionalPropertyEntry(
IcebergConstants.TABLE_METADATA_CACHE_CAPACITY,
"Table metadata cache capacity",
false /* immutable */,
- 200 /* defaultValue */,
+ 1000 /* defaultValue */,
Review Comment:
The default cache implementation is hard-coded as a string literal here,
which can drift from `IcebergConfig` over time (e.g., refactors/relocations).
Prefer using `LocalTableMetadataCache.class.getName()` (or a shared constant)
to keep defaults consistent and refactor-safe.
##########
docs/iceberg-rest-service.md:
##########
@@ -525,8 +525,8 @@ Gravitino features a pluggable cache system for updating or
retrieving table met
| Configuration item | Description
| Default value | Required | Since Version |
|--------------------------------------------------------------|---------------------------------------------|---------------|----------|---------------|
-| `gravitino.iceberg-rest.table-metadata-cache-impl` | The implement
of the cache. | (none) | No | 1.1.0 |
-| `gravitino.iceberg-rest.table-metadata-cache-capacity` | The capacity
of table metadata cache. | 200 | No | 1.1.0 |
+| `gravitino.iceberg-rest.table-metadata-cache-impl` | The implement
of the cache. |
`org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache` | No
| 1.1.0 |
+| `gravitino.iceberg-rest.table-metadata-cache-capacity` | The capacity
of table metadata cache. | 1000 | No | 1.1.0 |
| `gravitino.iceberg-rest.table-metadata-cache-expire-minutes` | The expire
minutes of table metadata cache. | 60 | No | 1.1.0 |
Gravitino provides the build-in
`org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache` to store
the cached data in the memory. You could also implement your custom table
metadata cache by implementing the
`org.apache.gravitino.iceberg.common.cache.TableMetadataCache` interface.
Review Comment:
Fix typos/grammar in the docs: change 'The implement of the cache.' to 'The
implementation of the cache.' and 'build-in' to 'built-in'.
##########
docs/lakehouse-iceberg-catalog.md:
##########
@@ -220,8 +220,8 @@ Gravitino features a pluggable cache system for updating or
retrieving table met
| Configuration item | Description
| Default value | Required | Since Version |
|---------------------------------------|---------------------------------------------|---------------|----------|---------------|
-| `table-metadata-cache-impl` | The implement of the cache.
| (none) | No | 1.1.0 |
-| `table-metadata-cache-capacity` | The capacity of table metadata
cache. | 200 | No | 1.1.0 |
+| `table-metadata-cache-impl` | The implement of the cache.
| `org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache` |
No | 1.1.0 |
+| `table-metadata-cache-capacity` | The capacity of table metadata
cache. | 1000 | No | 1.1.0 |
| `table-metadata-cache-expire-minutes` | The expire minutes of table metadata
cache. | 60 | No | 1.1.0 |
Gravitino provides the build-in
`org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache` to store
the cached data in the memory. You could also implement your custom table
metadata cache by implementing the
`org.apache.gravitino.iceberg.common.cache.TableMetadataCache` interface.
Review Comment:
Fix typos/grammar in the docs: change 'The implement of the cache.' to 'The
implementation of the cache.' and 'build-in' to 'built-in'.
##########
iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ops/IcebergCatalogWrapper.java:
##########
@@ -457,11 +457,13 @@ private TableMetadataCache
loadTableMetadataCache(IcebergConfig config, Catalog
return TableMetadataCache.DUMMY;
}
- Preconditions.checkArgument(
- catalog instanceof SupportsMetadataLocation,
- "You shouldn't enable Iceberg metadata cache for the catalog %s,"
- + " because the catalog impl does not support get metadata
location.",
- catalog.name());
+ if (!(catalog instanceof SupportsMetadataLocation)) {
+ LOG.warn(
+ "Skip Iceberg table metadata cache for catalog {} because the
catalog does not support"
+ + " metadata location.",
+ catalog.name());
+ return TableMetadataCache.DUMMY;
+ }
Review Comment:
The behavior changed from throwing (`Preconditions.checkArgument`) to
returning `TableMetadataCache.DUMMY` for catalogs without
`SupportsMetadataLocation`. Please add a unit test that exercises
`loadTableMetadataCache` with a catalog not implementing
`SupportsMetadataLocation` to assert it no longer throws and returns `DUMMY`
(and ideally that it logs once).
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogPropertiesMetadata.java:
##########
@@ -127,13 +127,14 @@ public class IcebergCatalogPropertiesMetadata extends
BaseCatalogPropertiesMetad
IcebergConstants.TABLE_METADATA_CACHE_IMPL,
"Table metadata cache implementation",
false /* immutable */,
- null /* defaultValue */,
+
"org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache"
+ /* defaultValue */,
Review Comment:
The default cache implementation is hard-coded as a string literal here,
which can drift from `IcebergConfig` over time (e.g., refactors/relocations).
Prefer using `LocalTableMetadataCache.class.getName()` (or a shared constant)
to keep defaults consistent and refactor-safe.
##########
docs/iceberg-rest-service.md:
##########
@@ -525,8 +525,8 @@ Gravitino features a pluggable cache system for updating or
retrieving table met
| Configuration item | Description
| Default value | Required | Since Version |
|--------------------------------------------------------------|---------------------------------------------|---------------|----------|---------------|
-| `gravitino.iceberg-rest.table-metadata-cache-impl` | The implement
of the cache. | (none) | No | 1.1.0 |
-| `gravitino.iceberg-rest.table-metadata-cache-capacity` | The capacity
of table metadata cache. | 200 | No | 1.1.0 |
+| `gravitino.iceberg-rest.table-metadata-cache-impl` | The implement
of the cache. |
`org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache` | No
| 1.1.0 |
+| `gravitino.iceberg-rest.table-metadata-cache-capacity` | The capacity
of table metadata cache. | 1000 | No | 1.1.0 |
| `gravitino.iceberg-rest.table-metadata-cache-expire-minutes` | The expire
minutes of table metadata cache. | 60 | No | 1.1.0 |
Gravitino provides the build-in
`org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache` to store
the cached data in the memory. You could also implement your custom table
metadata cache by implementing the
`org.apache.gravitino.iceberg.common.cache.TableMetadataCache` interface.
Review Comment:
Fix typos/grammar in the docs: change 'The implement of the cache.' to 'The
implementation of the cache.' and 'build-in' to 'built-in'.
##########
docs/lakehouse-iceberg-catalog.md:
##########
@@ -220,8 +220,8 @@ Gravitino features a pluggable cache system for updating or
retrieving table met
| Configuration item | Description
| Default value | Required | Since Version |
|---------------------------------------|---------------------------------------------|---------------|----------|---------------|
-| `table-metadata-cache-impl` | The implement of the cache.
| (none) | No | 1.1.0 |
-| `table-metadata-cache-capacity` | The capacity of table metadata
cache. | 200 | No | 1.1.0 |
+| `table-metadata-cache-impl` | The implement of the cache.
| `org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache` |
No | 1.1.0 |
+| `table-metadata-cache-capacity` | The capacity of table metadata
cache. | 1000 | No | 1.1.0 |
| `table-metadata-cache-expire-minutes` | The expire minutes of table metadata
cache. | 60 | No | 1.1.0 |
Gravitino provides the build-in
`org.apache.gravitino.iceberg.common.cache.LocalTableMetadataCache` to store
the cached data in the memory. You could also implement your custom table
metadata cache by implementing the
`org.apache.gravitino.iceberg.common.cache.TableMetadataCache` interface.
Review Comment:
Fix typos/grammar in the docs: change 'The implement of the cache.' to 'The
implementation of the cache.' and 'build-in' to 'built-in'.
--
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]
