Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24520 )
Change subject: [wip] [java] Tighten field visibility and thread-safety ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/RowAction.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/RowAction.scala: http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/RowAction.scala@38 PS2, Line 38: def fromValue(value: Byte): RowAction = byValue.getOrElse(value, null) nit: Returning `null` from Scala code is idiomatic Java but un-idiomatic Scala. Since the sole caller (`KuduRestore.scala`) pattern-matches the result and has a wildcard `case _` arm, this works - but `Option[RowAction]` would be the Scala-idiomatic return type. Understand this is a direct port of the Java behavior for minimal churn, so not blocking. http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@263 PS2, Line 263: private volatile long numRowsReturned = 0; The comment says `+=` is safe because only a single thread advances the scanner. But this is a non-atomic read-modify-write of a volatile long. If the single-writer guarantee ever breaks (e.g., timeout/close racing with the scan callback), this becomes a lost-update bug with no compile-time or runtime warning. Consider whether an `AtomicLong` would be a safer expression of the same intent - it makes the single-writer assumption explicit without relying on it for correctness. (Low priority - the single-writer contract appears well-established.) http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java: http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@938 PS2, Line 938: private volatile long operationSize; q: `operationSize` is now both `volatile` AND accessed under `synchronized (monitor)` in `addOperation`/`bufferSize`. The `volatile` is only load-bearing for `toString()` which reads it without the lock. Was this intentional? If `toString` is meant to be best-effort diagnostic, this is fine; but if the intent is that all production reads go through the lock, documenting that would prevent someone later removing the "redundant" volatile and introducing a subtle race in toString. http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@945 PS2, Line 945: synchronized (monitor) { : operations.add(operation); : operationSize += operation.getOperation().getRow().size(); : } q: `addOperation` and `bufferSize` are now synchronized on `monitor`, but the other Buffer methods - `iterator()`, `isEmpty()`, `numOps()` - still access `operations` without the lock. Is the Buffer guaranteed to only be accessed through the outer class's already-synchronized paths for those methods? If so, annotating them `@GuardedBy("monitor")` would document the contract. http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java: http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@168 PS2, Line 168: private volatile long sequenceId = RequestTracker.NO_SEQ_NO; q: `sequenceId` is read and written in `reuse()` (which resets it to `NO_SEQ_NO` under no lock), `setSequenceId()` (which sets it, also no lock), and `getSequenceId()` / `toString()` (reads, no lock). Are there ordering guarantees between `setSequenceId` and the subsequent `getSequenceId` read in `RpcProxy.sendRpc`? Volatile gives visibility but if two threads could race between `setSequenceId` and `reuse`, there's still a possible hazard. (Likely fine given the RPC lifecycle, but worth confirming the happens-before chain is intentional.) http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@170 PS2, Line 170: synchronized int nextAttempt() { q: These accessors synchronize on `this` (the KuduRpc instance). In the hot path - particularly `getSleepTimeForRpcMillis` and `cannotRetryRequest` - every call to `getAttempt()` now acquires a monitor. Prior code used an unsynchronized read which was racy, but effectively read-only in those paths. Has there been any measurement of the overhead of synchronizing on every retry check? For a low-contention field that is only incremented by one thread at a time, an `AtomicInteger` or even just `volatile int` with a non-atomic increment (since only one thread increments) might be a lighter-weight fix that still satisfies SpotBugs. Not blocking - just a design consideration. http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java: http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTransaction.java@163 PS2, Line 163: private volatile boolean isCommitStarted = false; nit: `isCommitStarted` is always read and written inside `synchronized (isCommitStartedSync)`. The `volatile` is fully redundant here - the lock already provides visibility. Harmless, but removing it would be more consistent with the fact that `volatile` on the other fields actually serves a purpose (they have unguarded reads). -- To view, visit http://gerrit.cloudera.org:8080/24520 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0b96c85c4a42d03d5b7911a823a147c0b66471f1 Gerrit-Change-Number: 24520 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Mon, 29 Jun 2026 15:59:44 +0000 Gerrit-HasComments: Yes
