Hongjiang Zhang 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: (7 comments) 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 Ack 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 Sure. Every time the client calls nextRows(), a new RPC together with a prefetching RPC are sent to tserver side, so the queue will not have more than 2 items. 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 ty Sure 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 Let me remove this internal 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 If there are 2 elements, the client API will continune calling nextRows() because hasMore() always returns true until the queue is drained. 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 exp Ack 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)? Yes, it should throw exception. -- 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: Wed, 18 Aug 2021 00:35:45 +0000 Gerrit-HasComments: Yes
