Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17388 )
Change subject: [backup] KUDU-3183 Add tablePrefix option to the Kudu restore job Adding a couple of options of the Kudu Restore job: ...................................................................... Patch Set 4: (8 comments) Thanks for updating the behavior! http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG Commit Message: PS4: Same nit feedback as those around the Options. http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG@7 PS4, Line 7: [backup] KUDU-3183 Add tablePrefix option to the Kudu restore job : Adding a couple of options of the Kudu Restore job: nit: put these on the same line http://gerrit.cloudera.org:8080/#/c/17388/4/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/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@57 PS4, Line 57: fullTableName.startsWith("impala::") nit: maybe store these in local variables; at least the ones that are reusable. Then we can simplify the logic a bit to something like E.g. var databaseName = "" var impalaPrefixLen = fullTableName.startsWith("impala::") ? 8 : 0 var indexOfPeriod = fullTableName.indexOf('.') // returns -1 if doesn't it exist; we can also use (indexOfPeriod == -1) instead of String.contains('.') if (indexOfPeriod == -1) { databaseName = fullTableName.substr(impalaPrefixLen, indexOfPeriod) onlyTableName = fullTableName.substr(indexOfPeriod + 1) } else { onlyTableName = fullTableName.substr(impalaPrefixLen) } // use the above variables to build the right restore name http://gerrit.cloudera.org:8080/#/c/17388/4/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/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@238 PS4, Line 238: "If set, replaces the existing database name and if there is no existing database name, a new database name " + nit: maybe we should explicitly mention that if not set, or set to "", this doesn't change the database name? In case users think it removes the db name. http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@239 PS4, Line 239: For E nit: either "For example" or "E.g." http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@240 PS4, Line 240: y nit: add a comma http://gerrit.cloudera.org:8080/#/c/17388/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@240 PS4, Line 240: be having nit: "will have" http://gerrit.cloudera.org:8080/#/c/17388/4/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/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@801 PS4, Line 801: val options = createRestoreOptions(Seq(tableName)) Do we have any remaining test coverage for if the new database isn't set? Perhaps it's worth leaving the default empty and adding an explicit test for both adding and changing the database name? -- 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: 4 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 20 May 2021 21:00:22 +0000 Gerrit-HasComments: Yes
