dramaticlly opened a new pull request, #17627: URL: https://github.com/apache/iceberg/pull/17627
### 1. Remove five orphaned classes `S3SignRequest`, `S3SignResponse`, `S3SignRequestParser`, `S3SignResponseParser`, `S3ObjectMapper`. Zero risk: verified the only references to any of the five are from each other. `S3V4RestSignerClient` already uses the `RemoteSign*` equivalents and no test source touches them. ### 2. Remove the deprecated property fallbacks in `S3V4RestSignerClient` - `s3.signer.uri` (`S3_SIGNER_URI`) → `RESTCatalogProperties.SIGNER_URI` - `s3.signer.endpoint` (`S3_SIGNER_ENDPOINT`) → `RESTCatalogProperties.SIGNER_ENDPOINT` - `S3_SIGNER_DEFAULT_ENDPOINT` (`"v1/aws/s3/sign"`), which had no replacement This also resolves the three `// TODO remove in 1.12.0` / `// TODO change to required in 1.12.0` markers already in the source. ### ⚠️ Behavior change — please confirm `RESTCatalogProperties.SIGNER_ENDPOINT` is now **required** instead of defaulting to `"v1/aws/s3/sign"`. Remote signing configured without an explicit signer endpoint previously logged a warning and used the default; it now fails at construction with `IllegalArgumentException`. Concretely, anyone setting `s3.remote-signing-enabled=true` without `rest.signer.endpoint` breaks on upgrade. That is what the `check()` TODO scheduled, but it is stricter than a pure deprecation removal — if you would rather keep the default for one more release, I can drop the `required` half and keep only the legacy-property removal. Three existing tests needed `SIGNER_ENDPOINT` added for exactly this reason (`TestS3FileIOProperties` ×2, which exercise `applySignerConfiguration`). The `S3RestSigner` integration test already sets it explicitly and is unaffected. New coverage: `TestS3V4RestSignerClient.signerEndpointIsRequired`. The `legacySignerProperties` parameterized test became `signerUriResolution`, since precedence between legacy and current properties no longer exists. One thing to note: `endpoint()` keeps a `Preconditions` check that `check()` already enforces at construction, so it is unreachable in practice. I left it as defensive cover on a `@Value.Lazy` accessor rather than reshape more code in a behavior-changing PR — happy to drop it if you prefer. Split out of #16449 to reduce reviewer burden. No `.palantir/revapi.yml` entries: the `aws` module is not covered by the revapi baseline. ## AI Disclosure Model: Claude Opus 5 (1M context) Platform/Tool: Claude Code Human Oversight: reviewed Prompt Summary: split #16449 into smaller self-contained PRs; verify each group compiles and tests green standalone -- 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]
