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 11: (8 comments) http://gerrit.cloudera.org:8080/#/c/17773/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17773/11//COMMIT_MSG@24 PS11, Line 24: This error may cause the spark task to fail sometimes. > nit: I haven't gone through the entire patch yet, but is this addressed in This description is confusing. In fact, this issue is addressed by this patch. http://gerrit.cloudera.org:8080/#/c/17773/11/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/11/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@292 PS11, Line 292: > nit: we typically indent at four spaces for line wraps Ack http://gerrit.cloudera.org:8080/#/c/17773/11/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@299 PS11, Line 299: * This error may cause the spark task to fail sometimes. > nit: this detail doesn't seem important -- it seems like this would affect Ack http://gerrit.cloudera.org:8080/#/c/17773/11/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@449 PS11, Line 449: return this.hasMore || cachedPrefetcherDeferred.get() != null; > nit: now it's a bit confusing to have both the concept of "having more" and Ack http://gerrit.cloudera.org:8080/#/c/17773/11/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@654 PS11, Line 654: > nit: sam here with regards to spacing Ack http://gerrit.cloudera.org:8080/#/c/17773/11/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@671 PS11, Line 671: Deferred<RowResultIterator> prefetcherDeferred = client.scanNextRows(AsyncKuduScanner.this) : .addCallbacks(gotNextRow, nextRowErrback()); : if (!cachedPrefetcherDeferred.compareAndSet(null, prefetcherDeferred)) { : LOG.info("Skip one prefetching because two consecutive prefetching scan occurs"); : } > Rephrasing Alexey's question, once we call client.scanNextRows(), does it s client.scanNextRows() calls immediately sends a scan request. If we drop the request's deferred, I don't think there is any rows missing. http://gerrit.cloudera.org:8080/#/c/17773/11/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@1212 PS11, Line 1212: if (!(prefetching && closed)) { > nit: before entering the switch, would it make sense to check canBeIgnored( Ack http://gerrit.cloudera.org:8080/#/c/17773/11/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@1252 PS11, Line 1252: if (error != null) { : if (canBeIgnored(resp.getError().getCode())) { : LOG.info("Ignore false alert of scanner not found for scan request"); : error = null; : } : } > Would it make sense to put this up by the switch statement so we don't have Ack -- 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: 11 Gerrit-Owner: Hongjiang Zhang <hongjizh...@ebay.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hongjiang Zhang <hongjizh...@ebay.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 29 Sep 2021 09:18:14 +0000 Gerrit-HasComments: Yes
