laskoviymishka commented on code in PR #3377:
URL: https://github.com/apache/iceberg-rust/pull/3377#discussion_r4286732245
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -766,6 +839,7 @@ impl RestSessionCatalog {
}
let request = HttpRequest::build(request_builder)?;
+ client.invalidate_table(context, table);
Review Comment:
Nothing pins this ordering yet. Every mutation mock returns a clean status,
so moving `invalidate_table` back below `query_catalog(...)?` keeps the suite
green. I'd add one case where the drop errors out of `query_catalog` itself,
then assert that the next load sends no `If-None-Match`. A 5xx won't catch it,
because that's still `Ok(response)`.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -65,6 +68,31 @@ pub const REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str =
"disable-header-red
/// if a `token`, `credential` or `oauth2-server-uri` is configured, `none`
/// otherwise.
pub const REST_CATALOG_PROP_AUTH_TYPE: &str = "rest.auth.type";
+/// Maximum number of loaded tables kept for freshness-aware loading (defaults
+/// to 100; 0 disables the cache). The cache is keyed by session id, so it only
+/// helps callers that reuse a [`SessionContext`], and a table changed through
+/// this catalog is invalidated only for the session that changed it.
Review Comment:
The credential skip is only explained on the private `TableCache` alias. For
a catalog that vends credentials on every load, this property does nothing, so
I'd say that here where users will see it.
```suggestion
/// this catalog is invalidated only for the session that changed it. Loads
that
/// vend short-lived storage credentials are never cached.
```
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -65,6 +68,31 @@ pub const REST_CATALOG_PROP_DISABLE_HEADER_REDACTION: &str =
"disable-header-red
/// if a `token`, `credential` or `oauth2-server-uri` is configured, `none`
/// otherwise.
pub const REST_CATALOG_PROP_AUTH_TYPE: &str = "rest.auth.type";
+/// Maximum number of loaded tables kept for freshness-aware loading (defaults
+/// to 100; 0 disables the cache). The cache is keyed by session id, so it only
+/// helps callers that reuse a [`SessionContext`], and a table changed through
+/// this catalog is invalidated only for the session that changed it.
+pub const REST_CATALOG_PROP_TABLE_CACHE_MAX_ENTRIES: &str =
"rest-table-cache.max-entries";
+/// How long a loaded table stays cached for freshness-aware loading, in
+/// milliseconds (defaults to 5 minutes; must be positive and at most 1000
years).
+pub const REST_CATALOG_PROP_TABLE_CACHE_EXPIRE_AFTER_WRITE_MS: &str =
+ "rest-table-cache.expire-after-write-ms";
+
+const TABLE_CACHE_MAX_ENTRIES_DEFAULT: u64 = 100;
+const TABLE_CACHE_EXPIRE_AFTER_WRITE_MS_DEFAULT: u64 = 5 * 60 * 1000;
+/// moka panics on a time-to-live beyond 1000 years.
+const TABLE_CACHE_EXPIRE_AFTER_WRITE_MAX: Duration = Duration::from_secs(1000
* 365 * 24 * 3600);
+
+/// Tables loaded with an `ETag`, keyed by session id and identifier. Loads
that
+/// vend short-lived storage credentials are not cached, since a `304` cannot
+/// refresh them.
+type TableCache = moka::sync::Cache<(String, TableIdent), (String, Table)>;
+
+/// Whether a `loadTable` config key carries a short-lived storage credential.
+fn is_short_lived_credential(key: &str) -> bool {
+ let key = key.to_ascii_lowercase();
+ key == S3_SESSION_TOKEN || key == GCS_TOKEN ||
key.starts_with(ADLS_SAS_TOKEN)
Review Comment:
I'd add a small table-driven test for this. Only the `s3.session-token` arm
runs today, through the vended-credentials tests. Use `gcs.oauth2.token`, an
`adls.sas-token.<account>` key and a mixed-case `S3.Session-Token` as
positives, and `s3.sse.key`, `s3.access-key-id` and `encryption.key-id` as
negatives, so last round's over-skip can't quietly come back.
--
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]
