Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12879 )
Change subject: [backup] Add initial incremental backup/restore support ...................................................................... Patch Set 4: (9 comments) 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. Done 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? I can remove it. I had used it but later removed it. 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? Done 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. Done 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 eq Done 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. Done 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 You can force an incremental by manually setting `fromMs` via the command line options. See `tableOptions.fromMs != 0`. 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 o Done 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); Done -- 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 13:36:24 +0000 Gerrit-HasComments: Yes
