Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17388 )
Change subject: [backup] KUDU-3183: Add --newDatabaseName and --removeImpalaPrefix options to restore job ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/17388/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17388/7//COMMIT_MSG@24 PS7, Line 24: newDBtest > Shouldn't this be newDB.test? Otherwise it's just changing the table name. Yes, these are wrong. Thought I fixed them already. Will update in the next patch http://gerrit.cloudera.org:8080/#/c/17388/7/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala: http://gerrit.cloudera.org:8080/#/c/17388/7/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@48 PS7, Line 48: /** > nit: I had a hard time parsing some of this logic and tried simplifying it Tested this code, looks good. Updating in the next patch. http://gerrit.cloudera.org:8080/#/c/17388/7/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala: http://gerrit.cloudera.org:8080/#/c/17388/7/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@238 PS7, Line 238: "If set, replaces the existing database name and if there is no existing database name, a new database name " + > nit: Move up to the text( line like the rest of the options. Done http://gerrit.cloudera.org:8080/#/c/17388/7/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@240 PS7, Line 240: E.g. > replace with "example" Done http://gerrit.cloudera.org:8080/#/c/17388/7/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/17388/7/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@594 PS7, Line 594: def testRemoveImpalaPrefixFlag() { > Nit: testTableNameChangeFlags given this includes testing removeImpalaPrefi Done http://gerrit.cloudera.org:8080/#/c/17388/7/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@830 PS7, Line 830: var onlyTableName = tableName > Can we just use KuduRestore.getRestoreTableName here given we manually vali Done -- To view, visit http://gerrit.cloudera.org:8080/17388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I65adcc1b3de0a8e1ac5b7f50a2d3a7036aa69421 Gerrit-Change-Number: 17388 Gerrit-PatchSet: 7 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 25 May 2021 21:58:03 +0000 Gerrit-HasComments: Yes
