SS-367 Support for fetching and consuming Vended Credentials - #6
SS-367 Support for fetching and consuming Vended Credentials#6patrickwwbutler wants to merge 8 commits into
Conversation
ublubu
left a comment
ublubu
left a comment
There was a problem hiding this comment.
I looked through it and it seemed reasonable.
Then I had Claude look at it, and it still seems reasonable?
There's one weird bit about the logic for re-fetching the FileIO after moving a table (to a different prefix). 🤷
| // A location change moves metadata/data to a new prefix that the refresh | ||
| // load's vended credentials do not cover, so it needs a post-commit reload. | ||
| let location_changed = existing_updates | ||
| .iter() | ||
| .any(|update| matches!(update, TableUpdate::SetLocation { .. })); | ||
|
|
||
| let table_commit = TableCommit::builder() | ||
| .ident(self.table.identifier().to_owned()) | ||
| .updates(existing_updates) | ||
| .requirements(existing_requirements) | ||
| .build(); | ||
|
|
||
| catalog.update_table(table_commit).await | ||
| let committed = catalog.update_table(table_commit).await?; | ||
| if location_changed { | ||
| // The new location has its own vended credentials; the reused FileIO is | ||
| // scoped to the old prefix, so reload the table to pick them up. | ||
| catalog.load_table(committed.identifier()).await | ||
| } else { | ||
| // The commit response carries no credentials. Reuse the FileIO from the | ||
| // refresh load above (not `self.table`, which is left untouched when the | ||
| // metadata is unchanged) so freshly vended credentials are not dropped. | ||
| Ok(committed.with_file_io(refreshed.file_io().clone())) | ||
| } |
There was a problem hiding this comment.
This is weird to me because it's the only significant logic outside io and catalog, which are both obviously related to vending credentials.
Claude thinks this logic should be folded into catalog and that do_commit might not be the only operation that needs this "get an updated FileIO for this table" code.
| let full = std::mem::take(buf); | ||
| self.get_storage(&full[0])? | ||
| .delete_stream(stream::iter(full).boxed()) | ||
| .await?; |
There was a problem hiding this comment.
Claude pointed out this per-batch await, which causes us to process only one prefix at a time.
I think it's be fine as-is. We're only ever writing to one table at a time, so unless there are multiple prefixes (for vending credentials) within a single table (is this "multi-cloud" tables?), I don't expect significant impact.
`taplo fmt --check` in the lint job rejects the single-line `features` array added for `native-tls`; it exceeds the 80-column width, so taplo expands it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `# v2` comment no longer matches the pinned commit now that the `v2` tag has moved, which zizmor flags as ref-version-mismatch. Repin to the v2.9.2 commit, matching apache/iceberg-rust main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The snapshots were never regenerated after the downstream commits that added `RequestAuthenticator`/`TokenProvider` to iceberg-catalog-rest and `DeltaWriter`/`PositionDeleteFileWriter` plus the `RollingFileWriterBuilder` schema parameter to iceberg, so `make check-public-api` fails. Regenerated with cargo-public-api 0.51.0 on nightly-2026-03-05, matching CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Primarily pulls the work from apache#2651 into our fork along with some fixes for conflicts to support catalogs that provide vended credentials for the storage containing iceberg tables.