Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24527 )
Change subject: KUDU-3790: Fix NPE in HdrHistogramAccumulator.isZero() ...................................................................... Patch Set 1: (2 comments) Just two questions, otherwise looks good. Thanks for fixing this issue! http://gerrit.cloudera.org:8080/#/c/24527/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala: http://gerrit.cloudera.org:8080/#/c/24527/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala@42 PS1, Line 42: override def isZero: Boolean = { q: If `histogram` can be null due to Kryo/unsafe-publication (the same scenario that motivates the isZero fix), then `copy()` will also NPE here. Same for `reset()` (line 57), `add()` (line 61), `merge()` (line 65), and `value` (line 69). Is this intentional scoping - i.e. those paths are only reachable after proper publication? It might be worth a one-line comment explaining why only isZero/toString need the null guard (e.g. "other methods are only called by the task-owning thread, which has happens-before with construction"). http://gerrit.cloudera.org:8080/#/c/24527/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala@86 PS1, Line 86: private[kudu] class HistogramWrapper(var innerHistogram: Option[IntCountsHistogram] = None) q: `innerHistogram` is a `var` read without synchronization in the new `isZero` (deliberate, per the comments). On x86 this works fine due to strong memory ordering, but on ARM-based executors the lack of `@volatile` means the JMM technically allows the reader to see a stale value indefinitely. Seeing stale-empty is harmless for the heartbeater (just reports zero), so this is fine in practice - but have you considered marking `innerHistogram` as `@volatile`? It would formalize the contract with minimal cost. -- To view, visit http://gerrit.cloudera.org:8080/24527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3d556aaf1051464303f35281d1d698b20d5ef20 Gerrit-Change-Number: 24527 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 30 Jun 2026 16:29:08 +0000 Gerrit-HasComments: Yes
