Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17773 )
Change subject: KUDU-1260: Fix prefetching bug on Java scanner ...................................................................... Patch Set 3: (8 comments) Did a quick first look. http://gerrit.cloudera.org:8080/#/c/17773/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17773/3//COMMIT_MSG@11 PS3, Line 11: or nit: or --> and http://gerrit.cloudera.org:8080/#/c/17773/3/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/17773/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@290 PS3, Line 290: here set capacity to 2. Could you provide more details on why it's so? Isn't it possible to get a higher number of elements in case of higher concurrency or other factors involved? http://gerrit.cloudera.org:8080/#/c/17773/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@295 PS3, Line 295: Deferred<RowResultIterator> nit: could drop this type specification since it can be deduced from the type of the 'cachedPrefetcherDeferred' variable? http://gerrit.cloudera.org:8080/#/c/17773/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@442 PS3, Line 442: @return nit: missing the description of the return value http://gerrit.cloudera.org:8080/#/c/17773/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@444 PS3, Line 444: private boolean hasMoreRowsInternal() { : return this.hasMore; : } Maybe, just use hasMore member instead of introducing this trivial private method? http://gerrit.cloudera.org:8080/#/c/17773/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@551 PS3, Line 551: return cachedPrefetcherDeferred.poll(); Since I see there is a comment about draining the queue, what if the queue got 2 elements at this point? http://gerrit.cloudera.org:8080/#/c/17773/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@649 PS3, Line 649: // Fix KUDU-1260 nit: I guess you can remove this once we know that prefetching works as expected and a test is added http://gerrit.cloudera.org:8080/#/c/17773/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@670 PS3, Line 670: LOG.error("Internal cached queue is overflow"); Does this manifest a data loss (i.e. dropping scanned rows on the floor)? If so, does it make sense the caller to know about that programmatically (e.g., throwing exception, etc.)? -- To view, visit http://gerrit.cloudera.org:8080/17773 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I853a041d86c75ec196d7d4ff45af4673c5c5f5cd Gerrit-Change-Number: 17773 Gerrit-PatchSet: 3 Gerrit-Owner: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 17 Aug 2021 19:45:57 +0000 Gerrit-HasComments: Yes
