Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18420 )
Change subject: [java] Fix a scan bug which will read repetitive rows. ...................................................................... Patch Set 15: (16 comments) Thank you for the fix! Overall looks good, just a few nits. http://gerrit.cloudera.org:8080/#/c/18420/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18420/15//COMMIT_MSG@12 PS15, Line 12: So a very offen very common scenarios nit: how about In common scenarios http://gerrit.cloudera.org:8080/#/c/18420/15//COMMIT_MSG@13 PS15, Line 13: when leader tserver restart nit: how about when tablet server hosting the leader replica restarts http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/main/java/org/apache/kudu/client/RowErrorsAndOverflowStatus.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowErrorsAndOverflowStatus.java: http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/main/java/org/apache/kudu/client/RowErrorsAndOverflowStatus.java@58 PS15, Line 58: rowErrors: { nit: why to duplicate the 'rowErrors' tag with for every element? Maybe, have it just once for all the elements in rowErrors? http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java File java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java: http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java@46 PS15, Line 46: tserver server nit: use either 'tablet server' or 'tserver', but not 'tserver server' http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java@48 PS15, Line 48: injection at nit: ... injection happens at ... http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java@48 PS15, Line 48: The nit: the http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java File java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java: http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@54 PS15, Line 54: 90000 Just curious: is it crucial to have 90000 instead of 20000 for some reason? Does it affect other test scenarios somehow? http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@83 PS15, Line 83: harness.getClient().isCreateTableDone(TABLE_NAME)) nit: why to add this? Maybe, just add one more retry instead just for the sake of clarity? http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@109 PS15, Line 109: The log for detail errors nit: how about Log on error details http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@192 PS15, Line 192: Injecting nit: Inject This and the following part of the sentence look a duplicate. Maybe, consolidate them? http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@195 PS15, Line 195: scan nit: scanning http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@195 PS15, Line 195: non-fault tolerant scanner nit: a non-fault tolerant scanner http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@195 PS15, Line 195: fault tolerant scanner nit: a fault tolerant scanner http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@217 PS15, Line 217: enableFaultInject nit: enableFaultInjection http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@237 PS15, Line 237: failureInjected nit: faultInjected ? http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@289 PS15, Line 289: assertTrue(threadPool.isShutdown()); What's the significance of this assertion? Could it ever happen that after calling threadPool.shutdown() and threadPool.awaitTermination() at lines 279,280 the pool isn't shutdown at this point? -- To view, visit http://gerrit.cloudera.org:8080/18420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d6be9df10c1a45cd971b52a0351028c1f5a023f Gerrit-Change-Number: 18420 Gerrit-PatchSet: 15 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Fri, 22 Apr 2022 05:55:18 +0000 Gerrit-HasComments: Yes
