[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/16201 )
Change subject: KUDU-1422 resize ErrorCollector ...................................................................... Patch Set 3: (4 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; > I don't see a reason for declaring maxCapacity as volatile, could you expla It's more of a memory visibility issue. maxCapacity is not final. It's initialised in the constructor which is not synchronised. There'll be no happens-before relation established between thread A calling the constructor and thread B calling other synchronised methods within the same object. http://gerrit.cloudera.org:8080/#/c/16201/2/java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java@104 PS2, Line 104: errorQueue.poll(); Do you guys think it'd better to actually "shrink" the size of arrayDeque when we resize down the maxCapacity? It's a memory optimisation since the amount of memory used by arrayDeque can only go up. http://gerrit.cloudera.org:8080/#/c/16201/2/java/kudu-client/src/main/java/org/apache/kudu/client/ErrorCollector.java@108 PS2, Line 108: maxCapacity = size; > I don't this this actually resizes anything. The error queue is already ini ArrayDeque is upper unnbounded. The capacity given in the constructor is its initial capacity. When it's necessary, it'll double its size on its own. so maxCapacity does control the max errors it can hold. http://gerrit.cloudera.org:8080/#/c/16201/2/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java File java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java: http://gerrit.cloudera.org:8080/#/c/16201/2/java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java@194 PS2, Line 194: I am not sure if it's fine to introduce a new method in the interface. -- 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: Thu, 16 Jul 2020 07:18:03 +0000 Gerrit-HasComments: Yes
