pan3793 commented on PR #8622: URL: https://github.com/apache/hadoop/pull/8622#issuecomment-5181319377
Reviewed `FSOutputSummer.java` together with the `DataChecksum` factory and checksum sizes. The fix correctly turns the overflow into a fast `IllegalArgumentException`, and the test is well-targeted (`238609295 * 9 = 2147483655 > Integer.MAX_VALUE`). Nothing in the repo catches `NegativeArraySizeException`, so the exception-type change is safe. Findings below are simplifications, not correctness issues. **1. Dead `if (bufSize < 0)` / `if (checksumBufSize < 0)` branches (minor)** `Math.multiplyExact` throws `ArithmeticException` on overflow and never returns a wrapped negative, and `DataChecksum.newDataChecksum` already rejects `bytesPerChecksum <= 0` (`DataChecksum.java:112`), so `bufSize` is always positive after a successful multiply. Both negative checks are unreachable and can be dropped. (The PR description mentions a single `Preconditions.checkArgument`, but the implementation uses `Math.multiplyExact` + try/catch + the dead checks.) **2. Checksum-buffer try/catch is dead (nit)** `getChecksumSize()` returns `type.size`, which is `0` (NULL) or `4` (CRC32/CRC32C) only -- `4 * 9 = 36` cannot overflow. The original one-liner `new byte[getChecksumSize() * BUFFER_NUM_CHUNKS]` was already safe. **3. `resetChecksumBufSize()` still uses raw multiplication (latent, minor)** `FSOutputSummer#resetChecksumBufSize` computes `sum.getBytesPerChecksum() * BUFFER_NUM_CHUNKS` without the overflow guard. Not reachable with an overflowing value today (the constructor now rejects them and `sum` is final), but the two paths for the same product are now inconsistent. Consider routing both through one `Math.multiplyExact` helper. **4. Exception type consistency (informational)** `DFSOutputStream`'s own validation throws `HadoopIllegalArgumentException`; the new plain `IllegalArgumentException` differs. Just noting the inconsistency, not a required change. -- 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]
