Abhishek Chennaka 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: (10 comments) http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG Commit Message: PS4: > Same nit feedback as those around the Options. Will do 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 Will do 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: > I thought the first line was a summary, and the second was the start of the Ah thanks for pointing that out. Will update the commit message http://gerrit.cloudera.org:8080/#/c/17388/4//COMMIT_MSG@11 PS4, Line 11: This will overwrite any existing : database and if there is no existing database > Kudu doesn't have a notion of a database of a standalone entity, so it's a Right, Kudu doesn't have the notion of a database. But in here we are assuming full table name consists of an optional database name and a table name.(fulltablename=databasename.tablename). Maybe I can clarify that in the commit message. Having a new table suffix or a new database name added would create a new table if the table is not present. In our case if we restore the table in the same cluster then we will be having both olddb.table and newdb.table with identical data. The backup/restore job will not affect the source tables. 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 reusa Makes sense. Thanks for the review. Will update this in the next patch set 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 Will do 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." Noted 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 Noted 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" Noted 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? P Yea we do not. Did this based on the how the table suffix test cases are implemented. Can reduce this down to one test case. -- 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: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 21 May 2021 00:55:25 +0000 Gerrit-HasComments: Yes
