Grant Henke 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: Code-Review+1 (7 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. http://gerrit.cloudera.org:8080/#/c/17388/7//COMMIT_MSG@24 PS7, Line 24: impala::newDB.default.test Shouldn't this also be impala::newDB.test? If we create a table that is impala::newDB.default.test then the table name changes to be "default.test" 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 a bit. Below is the code I wrote out. Feel free to use it if you think it's clearer. val ImpalaPrefix = "impala::" /** * Returns the table name in which the data will be restored considering the flags removeImpalaPrefix, * newDatabaseName and tableSuffix */ private def getRestoreTableName(fullTableName: String, options: RestoreOptions): String = { // Break the table down into prefix::databaseName.tableName var prefix = "" var databaseName = "" var tableName = fullTableName val hasImpalaPrefix = tableName.startsWith(ImpalaPrefix) if (hasImpalaPrefix) { prefix = ImpalaPrefix tableName = tableName.substring(ImpalaPrefix.length) } val hasDatabase = tableName.contains(".") if (hasDatabase) { databaseName = tableName.substring(0, tableName.indexOf(".") + 1) tableName = tableName.substring(tableName.indexOf(".") + 1) } // If the user does not want the Impala prefix, drop it if (options.removeImpalaPrefix) { prefix = "" } // If there is a databaseName specified by the user, use that if (options.newDatabaseName.nonEmpty) { databaseName = options.newDatabaseName.concat(".") } s"${prefix}${databaseName}${tableName}${options.tableSuffix}" } This matches closely to the code added in restoreAndValidateTable. I didn't run it so there could be some mistakes. 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. 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" 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 removeImpalaPrefix and newDatabaseName. 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 validate it in a different test above? -- 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 <achenn...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 25 May 2021 15:25:59 +0000 Gerrit-HasComments: Yes
