[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/16201 )
Change subject: KUDU-1422 resize ErrorCollector ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/16201/2/java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java File java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java: http://gerrit.cloudera.org:8080/#/c/16201/2/java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java@34 PS2, Line 34: private volatile int maxCapacity; > Even if there's no direct happens-before link between A and B, there is one All these behave like that given JVM synchronises an object during its construction. But to my knowledge it doesn't hold. JVM does't prevent you from doing something interesting, like publishing 'this' reference to other thread in the constructor. Other thread can do anything with that reference at any time. Because there is no internal synchronisation involved, 'this' object may be viewed in a inconsistent state by others, leading to unsafe publication. JVM internal synchronisation kicks in in other cases like executing static initialisers etc. The reason why Constructors can't be 'synchronised', I think , is each calling of a constructor will return a new object and there is no need for it to be 'synchronised'. But if you do need the synchronisation, actually you can put in a synchronised block to make a constructor synchronised. I agree with Greg Solovyev that once this object is safe published, then there is no need for volatile. Adding volatile, in my opinion, makes this class complete. http://gerrit.cloudera.org:8080/#/c/16201/2/java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java@89 PS2, Line 89: synchronized void resize(int size) { > shouldn't this be public? If you guys think exposing this method would be fine. I am ok with it. -- To view, visit http://gerrit.cloudera.org:8080/16201 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I53731f6367aa84d6435b3bf2143e86164c8eaa1e Gerrit-Change-Number: 16201 Gerrit-PatchSet: 3 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 18 Jul 2020 17:33:23 +0000 Gerrit-HasComments: Yes
