Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java: PS6: > I don't think physical type makes sense. That is primarily used server side Yeah I guess virtualType works. http://gerrit.cloudera.org:8080/#/c/12689/6/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/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@323 PS6, Line 323: if (startTimestamp != AsyncKuduClient.NO_TIMESTAMP) { > Not sure honestly...this was the existing behavior. I am not totally sure w Yeah I think that's worth investigating: 1. What does stripping achieve? 2. Why can we get away with not stripping these? Git history may help. http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@355 PS6, Line 355: .nullable(columnToClone.isNullable()) > Do you know why we need to strip the column schema? No clue; presumably if we pass along too much information back to the server, it complains? I took a look and the server will complain if the schema has column IDs in it, but the ColumnSchema Java class doesn't even store those. http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@244 PS6, Line 244: resultNumDeletes++; > It means a key was in the results of the scanner, but not in the original m I'd rephrase the comment to describe what this condition means w.r.t. the test's expectations. Something like "The key was not found in the mutations map. This means that we somehow managed to scan a row that was never mutated. It's an error and will trigger an assert below." http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@264 PS6, Line 264: private Map<Integer, ChangeType> generateMutations( > I can, this just seemed like a convenient place to contain a "session". Is Just less overhead, but it's not a huge deal if it'll contort the test. http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@325 PS6, Line 325: > INSERT will still be a valid endType, some mutations will occur on the orig I see where the disconnect is: I assumed that endType==INSERT means the row should not have been mutated. But in this context it actually means REINSERT; that is, this is a row whose final state is an "INSERT" because there was a DELETE right before it, and some number of UPDATE/DELETE/REINSERT before that. Maybe you can clarify that somewhere here with a comment? http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@337 PS6, Line 337: } > I still like to close sessions when I am done with them. I think it's a goo When you said closable you set off my spidey sense: how do you feel about opening the session via try-with-resources so that we'll still close it if something here throws? http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@347 PS6, Line 347: } : assertFalse(resp.hasRowError()); : return row.getInt(0); : } > I broke it out so that the error would be printed when the test fails, othe But don't assertion failures result in a displayed stack trace beginning with an AssertionError? And if you use the assertFalse(String, condition) variant as I suggested, won't that stack trace's AssertionError include the String? -- To view, visit http://gerrit.cloudera.org:8080/12689 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964 Gerrit-Change-Number: 12689 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Sat, 16 Mar 2019 04:40:52 +0000 Gerrit-HasComments: Yes
