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 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 p ...................................................................... Patch Set 6: (8 comments) 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: Wh > nit: drop Done 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: val impalaPrefixLen = if (fullTableName.startsWith("impala::")) 8 else 0 : val indexOfPeriod = fullTableName.indexOf('.') > nit: these can be vals since they're const Done 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 != Done 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.isEmpty) { > nit: prefer isEmpty(), since it's a cheaper operation. Same below. Done http://gerrit.cloudera.org:8080/#/c/17388/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@73 PS5, Line 73: impalaPrefixLen != 0 && (!options.re > nit: prefer impalaPrefixLen != 0, since it's a cheaper operation. Done http://gerrit.cloudera.org:8080/#/c/17388/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@72 PS5, Line 72: // Build the table name which is being restored and return : if (impalaPrefixLen != 0 && (!options.removeImpalaPrefix)) { : return s"impala::${databaseName}${onlyTableName}${options.tableSuffix}" : } : return s"${databas > nit: rather than storing a variable, how about something like: Done 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.isEmpty()) { : databaseName = options.newDatabaseName : } : if (!(databaseName.isEmpty())) { : databaseName = databaseName.concat(".") : } > nit: same here Done 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 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: 6 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: Mon, 24 May 2021 14:14:07 +0000 Gerrit-HasComments: Yes
