YuangGao commented on code in PR #3377:
URL: https://github.com/apache/iceberg-rust/pull/3377#discussion_r4291043624
##########
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:
done, thanks.
##########
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:
added a table-driven test with those positives and negatives.
##########
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:
added one: the drop's response is cut off mid-body, so it fails inside
query_catalog, and the next load sends no If-None-Match. it fails if the
invalidation moves back below the `?`.
--
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]
