You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Revanth14 (via GitHub)" <gi...@apache.org> on 2026/10/13 14:47:37 UTC
[PR] feat(rest): freshness-aware table loading with If-None-Match / 304 [iceberg-go]
Revanth14 opened a new pull request, #2188:
URL: https://github.com/apache/iceberg-go/pull/2188
Closes #2124.
## Problem
`rest.Catalog.LoadTable` always downloads and parses the full metadata JSON, even when the table has not changed. The REST spec lets `loadTable` send `If-None-Match` with the `ETag` from a previous `LoadTableResponse`, and lets the server answer `304 Not Modified`. Iceberg Java's `RESTTableCache` already does this.
## Fix
- **Cache:** a bounded, per-catalog cache maps each table identifier to its ETag and parsed load response. It is configured by Java's property names and defaults: `rest-table-cache.max-entries` (100; `0` disables) and `rest-table-cache.expire-after-write-ms` (5 minutes). Reads do not extend an entry's lifetime. Invalid values fail `NewCatalog`.
- **LoadTable:** on a cache hit, `LoadTable` sends `If-None-Match`. On a `304` it builds a new table from the cached response. A `200` with an `ETag` updates the cache. A `304` to a request that sent no ETag is still an error.
- **Invalidation (same points as Java):** a 404 on load, `DropTable`, `PurgeTable`, the rename source, `CheckTableExists` returning false, and `Close`.
## Testing
`table_cache_test.go` tests against an `httptest` server:
- a 304 reuses the cached response and returns a distinct table;
- a changed ETag replaces the entry;
- a response without an `ETag` is not cached;
- `max-entries=0` disables the cache;
- an unrequested 304 is rejected;
- each invalidation path clears the entry;
- invalid properties are rejected.
`table_cache_internal_test.go` checks expire-after-write with a fake clock, the entry limit, and key uniqueness.
Each test was confirmed to fail when the code it covers is removed. `go test -race ./...` and `go vet ./...` pass.
## Context
Java reference: `RESTTableCache`, plus `RESTSessionCatalog.loadTable` and its invalidation call sites, with tests in `TestFreshnessAwareLoading`. Lakekeeper sends `ETag` and answers `304` from v0.11.0.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): freshness-aware table loading with If-None-Match / 304 [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #2188:
URL: https://github.com/apache/iceberg-go/pull/2188
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): freshness-aware table loading with If-None-Match / 304 [iceberg-go]
Posted by "richiebachala (via GitHub)" <gi...@apache.org>.
richiebachala commented on PR #2188:
URL: https://github.com/apache/iceberg-go/pull/2188#issuecomment-6240876259
Thanks @Revanth14, verified against 76281de. I reran my repro on this commit, extended to mutate field labels as well as object labels, through both the first table and a cached one: each load now gets independent labels. Your tests and `go test -race ./catalog/rest` pass too.
Good catch on copying in `put` as well as on a hit. I'd only suggested the hit side, which would still have left the first load's table sharing the stored pointer. The mode check reads well too, and keeping the key to the identifier for one-entry invalidation makes sense.
LGTM from my side.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(rest): freshness-aware table loading with If-None-Match / 304 [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on PR #2188:
URL: https://github.com/apache/iceberg-go/pull/2188#issuecomment-6238453288
> Nice work: this tracks Java's `RESTTableCache` closely, the 304 path only opens when an ETag was actually sent, and the invalidation points match `RESTSessionCatalog`. I checked the design against Java and ran the tests; one bug and one small suggestion.
>
> **Bug: a 304 shares one `*iceberg.Labels` across tables.** `loadTableWithMode` passes `ret.Labels` from the cached response straight to `tableFromResponse`, so every load served from the cache gets the same pointer. `Labels` is mutable (`Table.Labels().Object()` returns the internal `ObjectLabels` map), so a mutation through one table is visible to every later load of that table. On `main` each load parses its own `Labels`, so this is new with the cache, and it undercuts the intent of the `tableCacheEntry` comment that callers never share state.
>
> Repro: load a table whose response carries `object-labels`, set `first.Labels().Object()["owner"] = "x"`, then load again (answered 304). `second.Labels() == first.Labels()`, and the second table sees `"x"`. The same check passes on `main` and fails on this branch.
>
> Java shares one `Labels` across tables on a 304 too, but there it is `@Value.Immutable`, so sharing is safe. The metadata side is already covered here, since `Snapshots()`, `Schemas()`, `Properties()` and the other accessors return copies. Deep-copying `Labels` on a cache hit would close the gap, with a test asserting the second table's labels are independent.
>
> **Suggestion: account for snapshot mode in the cache key.** `loadTableWithMode` takes a `mode`, but the key is only the identifier. Every caller passes `r.snapshotMode` today, so this is latent, but if a caller ever loads with a different mode, a `refs` response could be replayed for an `all` request on a 304 and silently drop snapshots. Including the mode in the key, or a comment noting the key assumes the catalog-wide mode, would guard it.
>
> Checked and fine:
>
> * Identifier-only keys are correct here: unlike Java's `SessionIdTableId`, a Go `Catalog` holds a single identity.
> * Tables built from a cached response still refresh vended credentials through `vendedCredentialRefresher`, matching Java's reuse of cached credentials on 304.
> * Metadata is parsed eagerly and only read on a hit.
> * `go test -race ./catalog/rest` passes.
Thanks, both fixed. The cache now copies `Labels` when it stores a response and again on every hit; copying only on hit isn't enough, since the first load's table shares the pointer `put` stores. `TestLoadTableNotModifiedDoesNotShareLabels` follows your repro. Each entry also records its snapshot mode, and a load with a different mode is treated as a miss (`TestTableCacheIgnoresEntryForOtherSnapshotMode`). I kept the key to the identifier so invalidation stays one entry per table.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org