aritragster commented on PR #4052: URL: https://github.com/apache/polaris/pull/4052#issuecomment-4483109667
@flyingImer @singhpk234 I've addressed all outstanding feedback, rebased on latest main, and filed the upstream issues. Here's the summary: ## Issues Filed - **Iceberg upstream:** https://github.com/apache/iceberg/issues/16399 — Expose `tableId` natively in `LoadTableResponse` (removes need for `ConfigCapturingHTTPClient` long-term) - **Polaris follow-up:** https://github.com/apache/polaris/issues/4486 — Refactor `StorageTypeFileIO` discriminator to support S3 Tables without opt-out ## Review Feedback Addressed **@singhpk234's feedback:** - `S3_TABLES` case moved before `GCS` in `PolarisStorageIntegrationProviderImpl` switch - `TABLE_ID_CONFIG_KEY` renamed to `S3TABLES_TABLE_ID_CONFIG_KEY` - Shared STS assume-role logic extracted to `AwsStsUtil` — both `AwsCredentialsStorageIntegration` (S3) and `AwsS3TablesCredentialsStorageIntegration` now use it, eliminating duplication - `ConfigCapturingHTTPClient` Javadoc made generic (removed S3 Tables-specific reference) - Rebased on latest main, all conflicts resolved **@flyingImer's feedback:** - TODO added on `ConfigCapturingHTTPClient` linking https://github.com/apache/iceberg/issues/16399 - TODO added on `StorageTypeFileIO` S3_TABLES entry linking https://github.com/apache/polaris/issues/4486 **Additional cleanup:** - Fixed `IcebergRESTFederatedCatalogFactory` constructor name mismatch (pre-existing bug exposed by rebase) - Removed dead inline ARN logic from `IcebergCatalogHandler` (already encapsulated in `S3TablesUtil.resolveTableLocations`) - Added `@Nullable` annotations on `AwsStsUtil` parameters - Fixed duplicate `CLIENT_REGION`/`refreshCredentialsEndpoint` setting in S3 integration (util handles STS path, else-block handles non-STS path) ## New Tests - `AwsStsUtilTest` — 12 tests covering assume-role behavior (parameters, credentials, expiration, region, refresh endpoint, session names, session tags) - `S3TablesUtilTest` — 9 tests covering ARN construction, validation, fail-closed behavior, and type detection ## On Sequencing with #3699 I've been tracking that PR. It currently has no formal approvals, an unresolved cache design discussion (`LoadingCache` vs `Supplier`-based Caffeine), and a pending dev ML vote on SPI removals. There's no clear timeline for it to land. This PR is self-contained and additive (~1,200 lines including tests, primarily new files). When #3699 lands with its `LocationGrant` model and `StorageIntegration` SPI, my S3 Tables integration becomes a clean new implementation of that interface — the `AwsS3TablesCredentialsStorageIntegration` maps directly to the new abstraction, and `AwsStsUtil` remains shared. I commit to doing that adaptation when #3699 merges. Merging this first doesn't add debt to the refactor path. It just means S3 Tables credential vending ships sooner. -- 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]
