Andrew Wong 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 24: (5 comments) Sorry for the delay! Looking better! http://gerrit.cloudera.org:8080/#/c/18420/24/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/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@83 PS24, Line 83: assertTrue(isDone || harness.getClient().isCreateTableDone(TABLE_NAME)); Why was this necessary? Doesn't the createTable() call wait for the table creation to complete by default? Or would it make sense to increase the timeout? I'm also curious why this change is necessary in the first place. It doesn't seem like this patch affects table creation. http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@94 PS24, Line 94: id != Integer.MIN_VALUE && What's special about MIN_VALUE here? http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@93 PS24, Line 93: int id = random.nextInt(); : if (id != Integer.MIN_VALUE && !primaryKeys.contains(id)) { : row.addInt(0, id); : primaryKeys.add(id); : } else { : i--; : continue; : } nit: I think it'd be a bit more straightforward to read this code as: int key = random.nextInt(); while (primaryKeys.contains(key)) { key = random.nextInt(); } row.addInt(0, key); primaryKeys.add(key); http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@204 PS24, Line 204: .batchSizeBytes(1) nit: please comment _why_ it's important for this test to have a small batch size. What client-side behavior are we trying to encourage? http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@279 PS24, Line 279: rowCount += result.get(); Should we check for -1 errors here too? -- 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: 24 Gerrit-Owner: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <acelyc1112...@gmail.com> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Wed, 04 May 2022 05:31:09 +0000 Gerrit-HasComments: Yes
