laskoviymishka commented on code in PR #2147:
URL: https://github.com/apache/iceberg-go/pull/2147#discussion_r4336958702
##########
table/properties.go:
##########
@@ -46,6 +46,33 @@ const (
ReadManifestListCacheEnabledKey = "read.manifest-list-cache.enabled"
ReadManifestListCacheEnabledDefault = true
+ // IOManifestCacheEnabledKey controls Java-compatible raw manifest-file
+ // content caching. These settings are FileIO/catalog configuration
rather
Review Comment:
Could we read these from the catalog props the way Java's FileIO does, or
document them as REST-only here and in configuration.md? `New()` builds the
cache from `savedConfig` alone
([table.go](https://github.com/apache/iceberg-go/blob/7dd63959affa37186b5fc61bb64d2deb293f6791/table/table.go#L1491)),
only REST sets it, and REST fills it from table properties plus the LoadTable
`config` while leaving out `r.props`
([rest.go](https://github.com/apache/iceberg-go/blob/7dd63959affa37186b5fc61bb64d2deb293f6791/catalog/rest/rest.go#L1840-L1846)).
So on REST this is effectively table metadata, and on Glue, SQL, Hive and
Hadoop it's silently off. A test enabling it through a catalog `LoadTable`
would pin whichever we pick.
##########
website/src/configuration.md:
##########
@@ -363,12 +363,28 @@ Property key constants are in
[`table/properties.go`](https://github.com/apache/
|---|---|---|
| `read.split.target-size` | `134217728` | Target size for coalescing safe
row-group ranges when splitting large local Parquet files. A range may exceed
the target when no supplied offset can divide it safely. |
| `read.manifest-list-cache.enabled` | `true` | Reuse decoded snapshot
manifest lists across scans of a table handle. Set to `false` to disable
retention and shared in-flight reads. Applied when loading or refreshing a
table, and when creating a transaction scan. |
+| `io.manifest.cache-enabled` | `false` | Reuse raw manifest file content
across local scans. This mirrors Java's FileIO/catalog setting and is read from
the table's saved catalog config. |
+| `io.manifest.cache.expiration-interval-ms` | `60000` | Idle expiration in
milliseconds. Zero disables time-based expiration; negative values disable the
cache. Expired entries are reclaimed on cache access/insertion, without a
background cleanup task. |
Review Comment:
Could this row say unparsable values disable the cache too? A non-numeric
expiration goes through the same parse branch as the size keys
([manifest_content_cache.go](https://github.com/apache/iceberg-go/blob/7dd63959affa37186b5fc61bb64d2deb293f6791/table/manifest_content_cache.go#L110-L123)),
but `TestManifestContentCacheInvalidNumericConfig` only loops over the two
size keys, so adding `IOManifestCacheExpirationIntervalMsKey` there would pin
it.
```suggestion
| `io.manifest.cache.expiration-interval-ms` | `60000` | Idle expiration in
milliseconds. Zero disables time-based expiration; negative or unparsable
values warn and disable the cache. Expired entries are reclaimed on cache
access/insertion, without a background cleanup task. |
```
##########
website/src/configuration.md:
##########
@@ -363,12 +363,28 @@ Property key constants are in
[`table/properties.go`](https://github.com/apache/
|---|---|---|
| `read.split.target-size` | `134217728` | Target size for coalescing safe
row-group ranges when splitting large local Parquet files. A range may exceed
the target when no supplied offset can divide it safely. |
| `read.manifest-list-cache.enabled` | `true` | Reuse decoded snapshot
manifest lists across scans of a table handle. Set to `false` to disable
retention and shared in-flight reads. Applied when loading or refreshing a
table, and when creating a transaction scan. |
+| `io.manifest.cache-enabled` | `false` | Reuse raw manifest file content
across local scans. This mirrors Java's FileIO/catalog setting and is read from
the table's saved catalog config. |
+| `io.manifest.cache.expiration-interval-ms` | `60000` | Idle expiration in
milliseconds. Zero disables time-based expiration; negative values disable the
cache. Expired entries are reclaimed on cache access/insertion, without a
background cleanup task. |
+| `io.manifest.cache.max-total-bytes` | `104857600` | Retained raw bytes per
independently loaded table. Handles returned by commit share their source
table's cache and budget. Concurrent population buffers and bytes held by
active readers are not counted. Non-positive or unparsable values warn and
disable caching. |
+| `io.manifest.cache.max-content-length` | `8388608` | Maximum size of one
manifest eligible for content caching. Compatible cache entries survive table
refresh and commit. Non-positive or unparsable values warn and disable caching.
|
Review Comment:
Could the survive-refresh sentence move to the paragraph below and say what
"compatible" means? It describes the whole cache, not this key, and
`reuseManifestContentCache` keeps the cache only when expiration, total bytes
and content length are all unchanged
([manifest_content_cache.go](https://github.com/apache/iceberg-go/blob/7dd63959affa37186b5fc61bb64d2deb293f6791/table/manifest_content_cache.go#L125-L136)).
```suggestion
| `io.manifest.cache.max-content-length` | `8388608` | Maximum size of one
manifest eligible for content caching. Non-positive or unparsable values warn
and disable caching. |
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
