pbajpai21 commented on PR #18050: URL: https://github.com/apache/iceberg/pull/18050#issuecomment-6037402575
> Thanks for adding these — direct coverage for `SerializationUtil` is a genuine gap (the only production callers are in `mr`, and nothing in `core` tested it directly), so having it is a small win. > > Most of the file round-trips through the JDK's own serialization and base64, which is fine but low-risk. The part I'd actually want tightened is the two `HadoopConfigurable` tests, since that branch is the one piece of logic unique to this class — and as written both pass without exercising it. `serializeToBytesAppliesCustomConfSerializerToHadoopConfigurable` returns the default `SerializableConfiguration` from its serializer, so it can't tell "our function's result was serialized" from "the util built its own and called the lambda incidentally"; and `hadoopConfigurableRoundTripPreservesConfiguration` round-trips fine even if the `instanceof HadoopConfigurable` branch were deleted, since the fixture wraps the conf in its constructor regardless. I left inline suggestions on both. > > The rest is minor — cause-based exception assertions over `hasMessage`, `\r\n` vs `\n`, the `(Object)` casts, and the `Test`-prefixed fixture name. None of those block. @laskoviymishka Thank you for the thorough review. These are very valuable and insightful comments. I have addressed all the comments. The two main ones: the Hadoop tests now genuinely fail if the `instanceof HadoopConfigurable` branch regresses (verified by temporarily removing it), and the `custom-serializer` test proves the serializer's output is what's serialized via a marker key. Details in the per-line replies. -- 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]
