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: [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]
