laskoviymishka commented on code in PR #3377:
URL: https://github.com/apache/iceberg-rust/pull/3377#discussion_r4252320713
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -1277,6 +1356,12 @@ impl SessionCatalog for RestSessionCatalog {
}
};
+ let vends_credentials = response
+ .storage_credentials
+ .as_ref()
+ .is_some_and(|credentials| !credentials.is_empty())
+ || response.config.keys().any(|key| is_sensitive_prop(key));
Review Comment:
`is_sensitive_prop` is a log-redaction denylist, not a credential
classifier, and reusing it here cuts both ways: it over-skips static keys like
`s3.sse.key` and `encryption.key-id` (those tables silently lose caching) and
can under-skip an oddly-named short-lived cred. I'd give the cache its own
predicate, ASCII-lowercased and scoped to the keys that actually carry
short-lived creds. The type-alias doc also says we skip when a load "vends
storage credentials", but the code skips on any sensitive-named config key —
worth lining the doc up with what it actually does.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -4023,6 +4115,214 @@ mod tests {
rename_table_mock.assert_async().await;
}
+ /// A `loadTable` body for `ns1.test1` whose `metadata-location` names
`tag`.
+ fn load_table_body(tag: &str) -> serde_json::Value {
+ let file = File::open(format!(
+ "{}/testdata/load_table_response.json",
+ env!("CARGO_MANIFEST_DIR")
+ ))
+ .unwrap();
+ let mut body: serde_json::Value =
serde_json::from_reader(BufReader::new(file)).unwrap();
+ body["metadata-location"] =
json!(format!("s3://warehouse/{tag}.metadata.json"));
+ body
+ }
+
+ /// Mocks one `loadTable` of `ns1.test1` that expects `if_none_match` and
+ /// answers `status`, carrying `etag` if given.
+ async fn load_table_mock(
+ server: &mut ServerGuard,
+ if_none_match: Matcher,
+ status: usize,
+ etag: Option<&str>,
+ ) -> Mock {
+ let mut mock = server
+ .mock("GET", "/v1/namespaces/ns1/tables/test1")
+ .match_header("if-none-match", if_none_match)
+ .with_status(status);
+ if let Some(etag) = etag {
+ mock = mock.with_header("etag", etag);
+ if status == 200 {
+ mock =
mock.with_body(load_table_body(etag.trim_matches('"')).to_string());
+ }
+ }
+ mock.create_async().await
+ }
+
+ fn test_table_ident() -> TableIdent {
+ TableIdent::from_strs(["ns1", "test1"]).unwrap()
+ }
+
+ #[tokio::test]
+ async fn test_load_table_revalidates_cached_table_with_etag() {
+ let mut server = Server::new_async().await;
+ let _config = create_config_mock(&mut server).await;
+ let _v1 = load_table_mock(&mut server, Matcher::Missing, 200,
Some("\"v1\"")).await;
+ let reuse_v1 = load_table_mock(&mut server, "\"v1\"".into(), 304,
None).await;
+ let catalog =
session_catalog(RestCatalogConfig::builder().uri(server.url()).build());
+ let context = SessionContext::empty();
+ let location = |table: Table|
table.metadata_location().unwrap().to_string();
+
+ let v1 = "s3://warehouse/v1.metadata.json";
+ let v2 = "s3://warehouse/v2.metadata.json";
+ let ident = test_table_ident();
+ let load = || catalog.load_table(&context, &ident);
+ assert_eq!(location(load().await.unwrap()), v1);
+ assert_eq!(location(load().await.unwrap()), v1);
+
+ reuse_v1.remove_async().await;
+ let _v2 = load_table_mock(&mut server, "\"v1\"".into(), 200,
Some("\"v2\"")).await;
+ let reuse_v2 = load_table_mock(&mut server, "\"v2\"".into(), 304,
None).await;
+ assert_eq!(location(load().await.unwrap()), v2);
+ assert_eq!(location(load().await.unwrap()), v2);
+ reuse_v2.assert_async().await;
+ }
+
+ #[tokio::test]
+ async fn test_load_table_not_modified_without_cached_table() {
+ let mut server = Server::new_async().await;
+ let _config = create_config_mock(&mut server).await;
+ let _load = load_table_mock(&mut server, Matcher::Missing, 304,
None).await;
+ let catalog =
session_catalog(RestCatalogConfig::builder().uri(server.url()).build());
+
+ let err = catalog
+ .load_table(&SessionContext::empty(), &test_table_ident())
+ .await
+ .unwrap_err();
+
+ assert_eq!(err.kind(), ErrorKind::Unexpected);
+ }
+
+ #[tokio::test]
+ async fn test_table_cache_is_per_session() {
+ let mut server = Server::new_async().await;
+ let _config = create_config_mock(&mut server).await;
+ let miss = load_table_mock(&mut server, Matcher::Missing, 200,
Some("\"v1\""))
+ .await
+ .expect(2);
+ let hit = load_table_mock(&mut server, "\"v1\"".into(), 304,
None).await;
+ let catalog =
session_catalog(RestCatalogConfig::builder().uri(server.url()).build());
+ let session = |id: &str|
SessionContext::builder().session_id(id.to_string()).build();
+
+ for id in ["a", "b", "a"] {
+ catalog
+ .load_table(&session(id), &test_table_ident())
+ .await
+ .unwrap();
+ }
+
+ miss.assert_async().await;
+ hit.assert_async().await;
+ }
+
+ #[tokio::test]
+ async fn test_table_cache_disabled_with_zero_max_entries() {
+ let mut server = Server::new_async().await;
+ let _config = create_config_mock(&mut server).await;
+ let load = load_table_mock(&mut server, Matcher::Missing, 200,
Some("\"v1\""))
+ .await
+ .expect(2);
+ let catalog = session_catalog(
+ RestCatalogConfig::builder()
+ .uri(server.url())
+ .props(HashMap::from([(
+ REST_CATALOG_PROP_TABLE_CACHE_MAX_ENTRIES.to_string(),
+ "0".to_string(),
+ )]))
+ .build(),
+ );
+ let context = SessionContext::empty();
+
+ catalog
+ .load_table(&context, &test_table_ident())
+ .await
+ .unwrap();
+ catalog
+ .load_table(&context, &test_table_ident())
+ .await
+ .unwrap();
+
+ load.assert_async().await;
+ }
+
+ #[tokio::test]
+ async fn test_drop_table_invalidates_cached_table() {
Review Comment:
Good to have drop covered — I'd extend the same load/mutate/load shape to
the other invalidation calls this round added: rename, register, commit and the
404 branch are all untested one-liners that regress silently. The `_ =>
invalidate` arm is the other gap: a cached entry whose next 200 starts vending
creds should evict, and nothing exercises that today — two credential-vending
200s both pass even if the arm were deleted, since nothing was ever cached. A
`storage_credentials` case alongside the config-key one would close it.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -767,6 +824,7 @@ impl RestSessionCatalog {
let request = HttpRequest::build(request_builder)?;
let http_response = client.query_catalog(context, request).await?;
+ client.invalidate_table(context, table);
Review Comment:
I'd move this to before we send the mutating request (or onto a path that
runs on error too): as written it sits after `query_catalog(...)?`, so a
transport error — the server applied the drop but we saw a timeout or reset —
takes the `?` and skips the invalidation, leaving the stale entry. That's the
ambiguous-outcome case, and the exact drop-and-recreate hazard from last round;
the `If-None-Match` check only saves us while the server never reuses an ETag.
Same ordering applies to rename, register and commit — invalidate is
idempotent, so running it on both paths is fine.
--
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]
