Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12879 )
Change subject: [backup] Add initial incremental backup/restore support ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/12879/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/12879/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@100 PS2, Line 100: // Generate an operation based on the row action. : val operation = rowAction match { : case RowAction.UPSERT => table.newUpsert() > > Does Scala allow returning tuples like in Python? OK, I was missing two things here: 1. There's an extra layer of indirection between this and the iterator in KuduBackupRDD, and the internal row is the only mechanism to pass data from one to the other. 2. More importantly, KuduBackupRDD is the read-side of the equation while this is the write-side; there's a persistence step in between, and we need to serialize the row action in order for it to survive the persistence. Next question: why bother converting is_deleted in KuduBackupRDD at all? Why not persist it as-is and convert it into a row action on-the-fly during restore? Then you won't need a persistence model for RowAction; it could be a simple enum, or maybe not exist at all (you could switch here on the value of the boolean is_deleted column). http://gerrit.cloudera.org:8080/#/c/12879/2/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/12879/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala@37 PS2, Line 37: tables: Seq[String], : rootPath: String, : kuduMasterAddresses: String = InetAddress.getLocalHost.getCanonicalHostName, > The CommonOptions trait is specifying that BackupOptions/RestoreOptions nee Hmm, not really seeing the point of the trait then, at least not in terms of reducing LOC. Is it used somewhere else in the patch that I missed? -- To view, visit http://gerrit.cloudera.org:8080/12879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I50b21d921fefbf4d7e8bd1766285258e8014d890 Gerrit-Change-Number: 12879 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Mon, 15 Apr 2019 19:17:10 +0000 Gerrit-HasComments: Yes
