pbajpai21 commented on PR #18050:
URL: https://github.com/apache/iceberg/pull/18050#issuecomment-6037731642
> Meaningful round-trip assertions (not tautological), the prior reviewer's
HadoopConfigurable concern was genuinely addressed, CI is fully green, no
flakiness. The findings below are nits/questions; none block.
>
> 1. Nit: `Base64.getMimeEncoder()` emits CRLF line breaks, so
`contains("\r\n")` would pin the actual MIME behavior more precisely than
`contains("\n")`.
>
> 2. One error path looks uncovered: `deserializeFromBase64` with
malformed input throws a raw `IllegalArgumentException` from the MIME decoder,
while the byte-path failures are wrapped in `UncheckedIOException`. Is that
asymmetry intentional? If so, a test documenting it would lock the contract in.
>
> 3. Minor: null is covered on both deserialize paths but not on
`serializeToBytes(null)` (which writes and reads back null cleanly). Worth a
one-liner for symmetry, or a note if intentionally omitted.
>
> 4. Nit: in
`serializeToBytesAppliesCustomConfSerializerToHadoopConfigurable`, the
single-threaded invocation flag works fine as a one-element array, but
`AtomicBoolean` is the more idiomatic choice.
@developer-rpai Thank you for your review and approval on PR.
I have applied `\r\n` and added a `serializeToBytes(null)` round-trip test,
the `boolean[]` flag is already gone from the reworked `HadoopConfigurable`
test and the base64 `IllegalArgumentException` is intentional - it's thrown by
the JDK's Base64 decoder before bytes reach `deserializeFromBytes`.
--
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]
