Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/10941 )
Change subject: Add error handling to KuduBackup ...................................................................... Patch Set 4: Code-Review-1 (16 comments) http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupStatus.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupStatus.scala: http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupStatus.scala@5 PS2, Line 5: > Since their isn't a ton of custom functionality perhaps using Scala's built I've removed this for now, will tackle it at a later time. Currently just throw an exception and tell the user to look back through the logs for failure. http://gerrit.cloudera.org:8080/#/c/10941/2/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/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@39 PS2, Line 39: @InterfaceStability.Unstable > Is there a reason you moved main to the top of the class? I think we should I've got the 'scalafmt' plugin in intellij and I did the formatting out of reflex. I'll fix the formatting/order http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@56 PS2, Line 56: .map { metadata => > Nit. Use a more specific exception. Done http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@57 PS2, Line 57: if (metadata.getToMs >= options.timestampMs) { > Instead of "at least one". Include the count that failed out of the total s Done http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@70 PS2, Line 70: } > I am not sure this is how we will handle incremental. I am thinking we woul I've changed this to check the timestamp in the metadata to see if the current timestamp is already backed up. If it is, mark it success. If it's not, the incremental can be handled there. If we failed when the metadata exists, and in a yarn job some tables succeeded and there was a retry, the tables that succeeded the first time would be marked failed on the retry. http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@74 PS2, Line 74: t: String, > Why did this context get move here? I think it can be shared for all uses i Done http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@87 PS2, Line 87: } > Maybe info level Done http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@96 PS2, Line 96: log.error(s"Failed to remove path: $tablePath", ex) > Nit: add explicit return type merged http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@127 PS2, Line 127: session.sqlContext.createDataFrame(rdd, sparkSchema(table.getSchema)) > Now that we do this 3 line preamble to get the filesystem and hpath a few t It's used twice, once in restore and once in backup (third time was removed). I'd like to leave it for now since the utility class + number of parameters in the method would be a little more complex than the few lines it takes right now. http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@135 PS2, Line 135: } > What is this for? It looks like it writes the metadata. removed. http://gerrit.cloudera.org:8080/#/c/10941/4/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/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@21 PS4, Line 21: import java.nio.file.{ Path, Paths } Is there a standard formatting tool you guys use? This is inconsistent with the other files, I'll fix this. http://gerrit.cloudera.org:8080/#/c/10941/1/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: PS1: Ignore this file. Restore will be more consistent with Backup. http://gerrit.cloudera.org:8080/#/c/10941/2/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/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@66 PS2, Line 66: val writeOptions = > Should the be RestoreSuccess/RestoreFailure? Since their isn't a ton of cus Done http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@79 PS2, Line 79: } > Nit: Explicit return type. Done http://gerrit.cloudera.org:8080/#/c/10941/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@91 PS2, Line 91: val hPath = new HPath(path.resolve(TableMetadata.MetadataFileName).toString) > Nit: Explicit return type Done http://gerrit.cloudera.org:8080/#/c/10941/4/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala: http://gerrit.cloudera.org:8080/#/c/10941/4/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala@94 PS4, Line 94: tmpDir = Files.createTempDirectory("backup") The 'backup' name doesn't make sense in this more global context. Will fix. -- 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: 4 Gerrit-Owner: Tony Foerster <anthonymfoers...@gmail.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tony Foerster <anthonymfoers...@gmail.com> Gerrit-Comment-Date: Thu, 19 Jul 2018 19:31:28 +0000 Gerrit-HasComments: Yes
