david-mollitor-db opened a new pull request, #58740:
URL: https://github.com/apache/spark/pull/58740
### What changes were proposed in this pull request?
`JavaUtils.checkArgument` and `JavaUtils.checkState` build their failure
message with
`String.format(msg, args)`, so `msg` is a format string and callers are
meant to pass runtime
values as trailing `args` (e.g. `checkArgument(cond, "bad key: %s", key)`).
This was not documented,
and one caller concatenated the value into the message instead. This PR:
- Expands the `checkArgument` / `checkState` JavaDoc to describe the
`String.format` contract and
warn against interpolating values into `msg`, showing the correct `%s`
idiom.
- Fixes the concatenating caller in
`RemoteBlockPushResolver.validateAndGetAppShuffleInfo`:
```java
// before
JavaUtils.checkArgument(appShuffleInfo != null,
"application " + appId + " is not registered or NM was restarted.");
// after
JavaUtils.checkArgument(appShuffleInfo != null,
"application %s is not registered or NM was restarted.", appId);
```
### Why are the changes needed?
Concatenating a value into `msg` is unsafe. On the failure path
`String.format` treats the
already-interpolated text as the format string, so a stray `%` in the value
(a percent-encoded
token, a Windows path such as `%TEMP%`, a SQL `LIKE` pattern, a number like
`"50%"`) makes it throw
an `IllegalFormatException` that replaces the intended
`IllegalArgumentException` and hides the real
reason the check failed. It also builds the message eagerly on every call,
including the common
success path where it is immediately discarded. Documenting the contract and
correcting the one
concatenating caller prevents this.
### Does this PR introduce _any_ user-facing change?
No. The message produced by the corrected caller is identical (`%s` is
substituted with `appId`),
and the remaining changes are documentation only.
### How was this patch tested?
Existing `RemoteBlockPushResolverSuite` passes (46 tests; the two tests that
assert the
"application ... is not registered" message still hold, since the produced
text is unchanged).
Checkstyle is clean on both `common-utils-java` and `network-shuffle`.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8
This pull request and its description were written by Isaac.
--
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]
