Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10941 )
Change subject: Add error handling to Backup and Restore ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/10941/6/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala: http://gerrit.cloudera.org:8080/#/c/10941/6/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@35 PS6, Line 35: private val cause: Throwable = None.orNull) nit: Just None.orNull is a bit unusual. Just null should work. http://gerrit.cloudera.org:8080/#/c/10941/6/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@46 PS6, Line 46: // TODO: Check if a vailid-looking result is in the directory and don't retry nit: s/vailid/valid http://gerrit.cloudera.org:8080/#/c/10941/6/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@57 PS6, Line 57: if (metadata.getToMs >= options.timestampMs) { I think we should only consider the case where getToMs == options.timestampMs right now. In that case we can skip the full backup. Otherwise we should fail saying a backup for a different time already exists (that should automatically happen if you fall through here). Then the user can delete it or provide a new path for the new backup. Because we don't support any idea of incremental yet, that seams the most straightforward to me. http://gerrit.cloudera.org:8080/#/c/10941/6/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@111 PS6, Line 111: val fs = hPath.getFileSystem(conf) nit: extra spaces http://gerrit.cloudera.org:8080/#/c/10941/6/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/10941/6/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@90 PS6, Line 90: val conf = session.sparkContext.hadoopConfiguration nit: extra spaces. I think a formatter is aligning the =. This should be fixed in general. http://gerrit.cloudera.org:8080/#/c/10941/6/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@103 PS6, Line 103: context: KuduContext): Try[KuduTable] = Try { nit: explicit return type. http://gerrit.cloudera.org:8080/#/c/10941/6/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@115 PS6, Line 115: private def dropTable(tableName: String, kuduClient: KuduClient): Try[Unit] = Where is this used? We definitely don't want the restore process dropping tables. I suppose we may want to drop a created table if it's restore failed for some reason. http://gerrit.cloudera.org:8080/#/c/10941/6/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@136 PS6, Line 136: s"${failures.length} were note restored, see previous logs for details") nit: s/note/not -- To view, visit http://gerrit.cloudera.org:8080/10941 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I104fd604e12b70fcff9beace71ed4ef96b54d691 Gerrit-Change-Number: 10941 Gerrit-PatchSet: 6 Gerrit-Owner: Tony Foerster <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tony Foerster <[email protected]> Gerrit-Comment-Date: Fri, 20 Jul 2018 20:42:30 +0000 Gerrit-HasComments: Yes
