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 While Kudu does not have a notion of database, usually the full table name is stored as <database>.<tablename> on kudu side. (NOTE: database name is optional). Using the options in this ...................................................................... Patch Set 5: Code-Review+1 (8 comments) Looking good, just more nits at this point. http://gerrit.cloudera.org:8080/#/c/17388/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17388/5//COMMIT_MSG@8 PS5, Line 8: nit: drop http://gerrit.cloudera.org:8080/#/c/17388/5/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/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@57 PS5, Line 57: var impalaPrefixLen = if (fullTableName.startsWith("impala::")) 8 else 0 : var indexOfPeriod = fullTableName.indexOf('.') nit: these can be vals since they're const http://gerrit.cloudera.org:8080/#/c/17388/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@59 PS5, Line 59: !(indexOfPeriod == -1) nit: can use != http://gerrit.cloudera.org:8080/#/c/17388/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@66 PS5, Line 66: newDatabaseName.equals("") nit: prefer isEmpty(), since it's a cheaper operation. Same below. http://gerrit.cloudera.org:8080/#/c/17388/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@73 PS5, Line 73: fullTableName.startsWith("impala::") nit: prefer impalaPrefixLen != 0, since it's a cheaper operation. http://gerrit.cloudera.org:8080/#/c/17388/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@72 PS5, Line 72: var restoreName: String = s"${databaseName}${onlyTableName}${options.tableSuffix}" : if (fullTableName.startsWith("impala::") && (!options.removeImpalaPrefix)) { : restoreName = s"impala::${databaseName}${onlyTableName}${options.tableSuffix}" : } : return restoreName nit: rather than storing a variable, how about something like: if (...has impala prefix) { return "impala::db.tablename/suffix" } return db.tablename/suffix http://gerrit.cloudera.org:8080/#/c/17388/5/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/5/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@839 PS5, Line 839: if (!options.newDatabaseName.equals("")) { : databaseName = options.newDatabaseName : } : if (!(databaseName.equals(""))) { : databaseName = databaseName.concat(".") : } nit: same here http://gerrit.cloudera.org:8080/#/c/17388/5/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestOptions.scala File java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestOptions.scala: http://gerrit.cloudera.org:8080/#/c/17388/5/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestOptions.scala@57 PS5, Line 57: k nit: capitalize Kudu -- 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: 5 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: Sat, 22 May 2021 05:25:26 +0000 Gerrit-HasComments: Yes
