Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12879 )
Change subject: [backup] Add initial incremental backup/restore support ...................................................................... Patch Set 4: Code-Review+1 (9 comments) Looks good. http://gerrit.cloudera.org:8080/#/c/12879/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupGraph.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupGraph.scala: http://gerrit.cloudera.org:8080/#/c/12879/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupGraph.scala@33 PS4, Line 33: private val adjacencyList = mutable.Map[Long, mutable.ListBuffer[BackupNode]]() Please add a comment the describing key -> val mapping. http://gerrit.cloudera.org:8080/#/c/12879/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupGraph.scala@34 PS4, Line 34: private val nodes = mutable.Set[Long]() Needs comment. Is 'nodes' used for anything? http://gerrit.cloudera.org:8080/#/c/12879/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupGraph.scala@37 PS4, Line 37: private val fullBackupFromMs = 0 style nit: this looks like a constant, so should be named with InitialCaps? http://gerrit.cloudera.org:8080/#/c/12879/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupGraph.scala@90 PS4, Line 90: adjacencyList(fromMs).flatMap { node => : allPaths(node.metadata.getToMs, path ++ List(node)) Wow, Scala is fancy. http://gerrit.cloudera.org:8080/#/c/12879/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupGraph.scala@149 PS4, Line 149: represents what the graph would look like : * at the passed point in time. suggestion: that represents the graph including only nodes with a ToTime equal to or less than the specified time http://gerrit.cloudera.org:8080/#/c/12879/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupGraph.scala@199 PS4, Line 199: def pathString: String = backups.map(_.metadata.getFromMs).mkString(" -> ") This is amazingly terse. Scala has built in method for everything. http://gerrit.cloudera.org:8080/#/c/12879/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/12879/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@66 PS4, Line 66: } else { We should have a --force-incremental to ensure that a backup administrator can guarantee some kind of SLA if they can't afford the resources to do a full backup and would rather get an error. OK to add this as a TODO item though. http://gerrit.cloudera.org:8080/#/c/12879/4/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/4/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@56 PS4, Line 56: val graph = io.readBackupGraph(tableName).filterByTime(options.timestampMs) What do you think about adding a TODO to add an option for an exact match on "to" timestamp instead of the best match based on timestamp? http://gerrit.cloudera.org:8080/#/c/12879/4/java/kudu-client/src/main/java/org/apache/kudu/Schema.java File java/kudu-client/src/main/java/org/apache/kudu/Schema.java: http://gerrit.cloudera.org:8080/#/c/12879/4/java/kudu-client/src/main/java/org/apache/kudu/Schema.java@206 PS4, Line 206: Integer index = this.columnsByName.get(columnName); : return index != null; nit: how about: return columnsByName.containsKey(columnName); -- 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: 4 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 16 Apr 2019 04:07:28 +0000 Gerrit-HasComments: Yes
