anxkhn opened a new pull request, #17110:
URL: https://github.com/apache/iceberg/pull/17110
<!-- BEGIN PR BODY -->
### What changed
`BaseCommitService.close()` throws an `IllegalArgumentException` via Guava
`Preconditions.checkArgument` when the commit pool does not finish within the
configured timeout (the rewrite/compaction commit-timeout path). The message
template used SLF4J-style `{}` placeholders:
```java
Preconditions.checkArgument(
!timeout && completedRewrites.isEmpty(),
"Timeout occurred when waiting for commits to complete. "
+ "{} file groups committed. {} file groups remain uncommitted. "
+ "Retry this operation to attempt rewriting the failed groups.",
committedRewrites.size(),
completedRewrites.size());
```
Guava's `Preconditions` message formatter only understands `%s`, not `{}`.
Because the placeholders never match, both `{}` are printed literally and the
two count arguments are appended in a trailing `[x, y]` bracket. Operators
who
hit a commit timeout therefore see a garbled diagnostic, for example:
```
Timeout occurred when waiting for commits to complete. {} file groups
committed.
{} file groups remain uncommitted. Retry this operation to attempt rewriting
the
failed groups. [0, 0]
```
### The fix
Replace the two `{}` with `%s` so the committed and uncommitted counts are
substituted inline. The two existing arguments (`committedRewrites.size()`,
`completedRewrites.size()`) already line up positionally with the two `%s`,
and
the wording is unchanged. The correct `{}` usage just above (the `LOG.warn`
at
`BaseCommitService.java:176`) is a genuine SLF4J call, which is the likely
source of the copy-paste.
### Test
Strengthens the existing regression test
`TestCommitService.testAbortFileGroupsAfterTimeout` so it guards this bug: in
addition to the existing `hasMessageContaining("Timeout occurred when waiting
for commits")`, it now asserts the message contains no literal `{}` and that
the
counts render inline (`N file groups committed. N file groups remain
uncommitted.`). Reverting only the source change makes the test fail on the
`{}` assertion; the fix makes it pass.
```
./gradlew :iceberg-core:spotlessCheck
./gradlew :iceberg-core:test --tests
"org.apache.iceberg.actions.TestCommitService"
```
both pass (2 tests, 0 failures).
### Notes
- `core/` only; no API change; wording preserved.
- Two other open PRs edit `BaseCommitService.java` (#16825 serializing
commits,
#16970 fixing the partial-commit failure count) but neither touches this
timeout message, so there is no line-level conflict.
---
**AI Disclosure**
- Model: Claude Opus 4.8
- Platform/Tool: opencode
- Human Oversight: fully reviewed
- Prompt Summary: Fix the rewrite commit-timeout IllegalArgumentException
message in BaseCommitService, which used SLF4J `{}` placeholders inside a Guava
`Preconditions.checkArgument` (which only supports `%s`), and add a regression
assertion.
<!-- END PR BODY -->
--
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]
