Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/24520 )
Change subject: [java] Tighten field visibility and thread-safety ...................................................................... Patch Set 4: (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 Sc Agreed it's not idiomatic Scala; kept null as a direct port for minimal churn since the caller pattern-matches with a wildcard arm. Left as-is for now. 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 sca Kept as-is: single-writer contract is well-established (only the scan-response path advances a given scanner) and there's an explicit comment documenting it. volatile covers cross-thread visibility of the count. 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: // All production reads/writes go through 'monitor' (see addOperation/bufferSize/resetUnlocked). > q: `operationSize` is now both `volatile` AND accessed under `synchronized Good catch, it's intentional. All production reads/writes go through monitor. The volatile is load-bearing only for the best-effort toString() read. Documented it so it isn't removed as "redundant" later. http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java@945 PS2, Line 945: ivate boolean flushNotificationFired = false; : : public void addOperation(BufferedOperation operation) { : s > q: `addOperation` and `bufferSize` are now synchronized on `monitor`, but t These are deliberately called without monitor on inactive buffers being flushed (doFlush, TabletLookupCB), so @GuardedBy("monitor") would be inaccurate and would flag those call sites. The active/inactive ownership model is documented on the class. Left unannotated. 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: * RPCs sent by this client. Once it is set it should never change unless the RPC is reused. > q: `sequenceId` is read and written in `reuse()` (which resets it to `NO_SE It is intentional: volatile gives visibility and the RPC lifecycle establishes the happens-before between setSequenceId/reuse and the getSequenceId read. No concurrent reuse hazard in practice. http://gerrit.cloudera.org:8080/#/c/24520/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java@170 PS2, Line 170: private volatile long sequenceId = RequestTracker.NO_SEQ_NO; > q: These accessors synchronize on `this` (the KuduRpc instance). In the hot Switched attempt to AtomicInteger, this removes the monitor on the retry-check paths, avoids locking on this, and still satisfies SpotBugs. Matches existing usage in the package. 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: // Always accessed under 'isCommitStartedSync', which already provides > nit: `isCommitStarted` is always read and written inside `synchronized (isC Right, isCommitStarted is always accessed under isCommitStartedSync. Dropped the volatile and added a note explaining why it differs from the sibling fields (which 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: 4 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 01 Jul 2026 11:53:53 +0000 Gerrit-HasComments: Yes
