Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support ...................................................................... Patch Set 6: (10 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: > Yeah I guess virtualType works. I opted for wireType, since that won't ever be "wrong" if the motivation for its use changes in the future. I included a comment about virtual columns in the docs. 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: columns.addAll(table.getSchema().getColumns()); > Yeah I think that's worth investigating: Stripping was added in this commit: https://github.com/apache/kudu/commit/7d57b8b9204a0b170fc204cb7aaa98ad995cf635 It looks like its primary, maybe only, purpose is to remove the isKey property from a column to allow for key columns to be any order in a scan. This seams like something the server side should allow regardless of isKey=true. I may introduce a patch to improve that some stripping columns can be removed. For now I will add a todo and update the function to strip the isKey only. http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@355 PS6, Line 355: private static ColumnSchema getStrippedColumnSchema(ColumnSchema columnToClone) { > No clue; presumably if we pass along too much information back to the serve See my comment above. http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java: http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@252 PS6, Line 252: // TODO: Move the logic that gets the indirectData index for String and Binary columns : // into a separate private method and enforce only INT64 and UNIXTIME_MICROS here. > It didn't feel appropriate for this patch. It can be done in a separate pat I fixed this in this patch. http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@698 PS6, Line 698: buf.append(TimestampUtil.timestampToString(getLong(i))); > Kind of surprised this doesn't operate on a java.sql.Timestamp (and call ge Done 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: PS6: > One thing that'd make the new test easier to read is if it separated its wo Done http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@244 PS6, Line 244: // This means the type was null, therefore the key was not found in the mutations map. > I'd rephrase the comment to describe what this condition means w.r.t. the t Done http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@325 PS6, Line 325: state.currentType == state.endType > I see where the disconnect is: I assumed that endType==INSERT means the row I moved this check to the top of the loop so that INSERTS can exist on their own too. http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@337 PS6, Line 337: session.close(); > When you said closable you set off my spidey sense: how do you feel about o I can't because KuduSession doesn't implement the "AutoCloseable" interface. It's likely because it's close method returns "List<OperationResponse>". http://gerrit.cloudera.org:8080/#/c/12689/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@347 PS6, Line 347: if (resp.hasRowError()) { : LOG.error("Could not insert row: " + resp.getRowError().getErrorStatus()); : } : assertFalse(resp.hasRowError()); > But don't assertion failures result in a displayed stack trace beginning wi You mean something like this: `assertFalse(resp.getRowError().getErrorStatus().toString(), resp.hasRowError()); ` The problem is the message is "aggressively" called regardless of failure and therefore results in a NPE because most often there isn't a row error. -- 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: 6 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: Mon, 18 Mar 2019 16:51:05 +0000 Gerrit-HasComments: Yes
