Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19384 )
Change subject: KUDU-1945: Support non unique primary key for Java client ...................................................................... Patch Set 20: (3 comments) Overall looks good to me! The only issues left are some nits pointed by LINT build: http://jenkins.kudu.apache.org/job/kudu-gerrit/27073/BUILD_TYPE=LINT/console An excerpt: 11:20:08 > Task :kudu-client:checkstyleMain 11:20:08 [ant:checkstyle] [WARN] /home/jenkins-slave/workspace/kudu-master/2/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:161:7: 'method def' child has incorrect indentation level 6, expected level should be 4. [Indentation] 11:20:08 [ant:checkstyle] [WARN] /home/jenkins-slave/workspace/kudu-master/2/java/kudu-client/src/main/java/org/apache/kudu/Schema.java:121:9: Local variable name 'KeyColumnCount' must match pattern '^[a-z]([a-z0-9][a-zA-Z0-9]*)?$'. [LocalVariableName] 11:20:08 [ant:checkstyle] [WARN] /home/jenkins-slave/workspace/kudu-master/2/java/kudu-client/src/main/java/org/apache/kudu/Schema.java:140:10: 'else' child has incorrect indentation level 9, expected level should be 10. [Indentation] 11:20:08 11:20:08 > Task :kudu-client:checkstyleMain FAILED 11:20:16 11:20:16 > Task :kudu-client:checkstyleTest 11:20:16 [ant:checkstyle] [WARN] /home/jenkins-slave/workspace/kudu-master/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:899:5: Distance between variable 'session' declaration and its first usage is 15, but allowed 3. Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance] http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@810 PS18, Line 810: assertTrue(schema.hasAutoIncrementingColumn()); : assertEquals(3, schema.getColumnCount()); > Now the column counts are same That's a great news. Thank you very much for updating that! http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java: http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2580 PS18, Line 2580: chema.isPrimaryKeyUnique()); > Updated code to have same behavior as C++ client. Thanks a lot! http://gerrit.cloudera.org:8080/#/c/19384/18/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java@2630 PS18, Line 2630: > Added a test case with a new table which has single auto-incrementing colum Ah, I see -- that makes sense. Thank you! -- To view, visit http://gerrit.cloudera.org:8080/19384 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7e2501d6b3d66f6466959e4f3f1ed0f5e08dfe5c Gerrit-Change-Number: 19384 Gerrit-PatchSet: 20 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Thu, 19 Jan 2023 19:23:52 +0000 Gerrit-HasComments: Yes
