viirya commented on PR #55028:
URL: https://github.com/apache/spark/pull/55028#issuecomment-4899835196
Thanks for working through all the rounds on this — the fix looks solid to
me. Two minor things worth cleaning up before merge, neither a correctness
blocker:
**Leftover non-ASCII in the test file.** Commit e10d2dce ("Remove non-ASCII
characters") only touched `GcmTransportCipher.java`, so the two em-dashes I
believe @HyukjinKwon flagged are still present at head:
- `GcmAuthEngineSuite.java:293` — `...silently dropped — which manifested
as...`
- `GcmAuthEngineSuite.java:610` — `...(32728, 32752] — test lower
boundary...`
CI won't catch these (the `nonascii` scalastyle rule only runs on Scala
sources, not Java), but they go against the ASCII-in-comments convention.
Replacing both `—` with `--` (or `-`) is all that's needed.
**The 1 MB bounded flush is untested, and one docstring now overstates the
guarantee.** The `plaintextAccumulator.readableBytes() >=
MAX_PLAINTEXT_BATCH_BYTES` flush branch added in 8c8ae52 is the subtlest new
piece of logic here, but nothing exercises it — crossing 1 MB needs ~32
segments and the largest existing case (`testSingleFirePerMessage`) is 5
segments (~160 KB), staying under the threshold. A regression test with a > 1
MB plaintext, asserting multiple `fireChannelRead` calls whose concatenation
round-trips to the original bytes, would lock in the batching behaviour.
Relatedly, `testSingleFirePerMessage`'s docstring says it fires "exactly one
`fireChannelRead` call regardless of the segment count" — that's no longer true
above 1 MB; worth rewording to scope it to the small-message case.
--
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]
