YuangGao commented on code in PR #3377:
URL: https://github.com/apache/iceberg-rust/pull/3377#discussion_r4253682161
##########
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:
moved the invalidation before the request on drop/purge, rename, register
and commit, so it also runs when the request fails in transit.
##########
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:
added a dedicated is_short_lived_credential: s3.session-token,
gcs.oauth2.token and adls.sas-token.*, ASCII-lowercased, the same keys Java
treats as short-lived. static keys like s3.sse.key are cached again, and the
doc now says short-lived credentials.
##########
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:
replaced the drop test with a load/change/load test across drop, rename,
register and commit, and added one for the 404 branch and one where a cached
entry's next 200 vends storage-credentials and gets evicted.
--
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]
