Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10899 )
Change subject: Test for backup/restore multiple tables ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/10899/1/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala File java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala: http://gerrit.cloudera.org:8080/#/c/10899/1/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@97 PS1, Line 97: one Nit: maybe rename this to "numRows" to assign it some kind of semantic meaning? http://gerrit.cloudera.org:8080/#/c/10899/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala: http://gerrit.cloudera.org:8080/#/c/10899/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala@111 PS1, Line 111: val bottom = schema.newPartialRow() // Unbounded. : val middle = schema.newPartialRow() : middle.addInt("key", 50) : val top = schema.newPartialRow() // Unbounded. Could you parameterize this so that createTable() can also be used to create the second table, on L107? http://gerrit.cloudera.org:8080/#/c/10899/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala@137 PS1, Line 137: table: KuduTable This is now shadowing the existing class-level 'table' variable; could you pick a different name here to avoid confusion? -- To view, visit http://gerrit.cloudera.org:8080/10899 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I439e0d1c6904395ad382e3f059846b3b03a79af4 Gerrit-Change-Number: 10899 Gerrit-PatchSet: 1 Gerrit-Owner: Tony Foerster <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 10 Jul 2018 21:06:23 +0000 Gerrit-HasComments: Yes
