Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17773 )
Change subject: KUDU-1260: Fix prefetching bug on Java scanner ...................................................................... Patch Set 16: (2 comments) Overall the code looks fine, but I have issues with how heavy-weight the test is. Seems we can and should reduce it into something much simpler, to reduce runtime, reduce potential flakiness, and overall make it easier to understand if something goes wrong in the test. http://gerrit.cloudera.org:8080/#/c/17773/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17773/11//COMMIT_MSG@10 PS11, Line 10: The writing thread records the timestamp of : its write, and the scanner thread creates two scanners (w and w/o prefetching), : by comparing the scan result of the two scanners, we can verify the : prefetching result. > I compared the results of the two scanners in order to debug the issue. If That makes it a good test for debugging purposes. However, for automated tests, which get run on every commit, it's more preferred to have smaller, more targeted tests that are less resource intensive, take less time, but still provide test coverage for the bug we're trying to fix. Before merging the test, could you reduce it to just the bare minimum required to test the prefetching behavior? 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@299 PS11, Line 299: * This error may cause the spark task to fail sometimes. > Ack Could you remove it? It doesn't seem to add useful information and therefore may confuse readers. -- 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: 16 Gerrit-Owner: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hongjiang Zhang <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 11 Oct 2021 21:15:02 +0000 Gerrit-HasComments: Yes
