Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10375 )

Change subject: Kudu Backup/Restore Spark Jobs
......................................................................


Patch Set 16:

(23 comments)

http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG@9
PS14, Line 9: Adds a rough base implementation of Kudu backup and restore
> Nit: please reformat the commit message so it adheres to our style guide's
Done


http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG@9
PS14, Line 9:
> Nit: todos
Done


http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG@13
PS14, Line 13:
> Nit: as written, you should combine these two sentences: "in any spark comp
Done


http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG@13
PS14, Line 13:
> In the "per table" directory, perhaps? It's a directory per table, right?
Done


http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG@15
PS14, Line 15: These jobs, as annotated, should be considered private, unstable,
> The second sentence is a fragment; consider joining it to the first: "...me
Done


http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:

http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/protobuf/backup.proto@28
PS12, Line 28:
> Can you move TableMetadataPB to the bottom?
Done


http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:

http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@22
PS14, Line 22: syntax = "proto3";
> The default in proto3 is for each field to be optional, right?
Explicit "optional" keyword are disallowed in proto3 syntax, as fields are
optional by default; required fields are no longer supported.

They also removed field presence logic for primitive value fields and default 
values. This is why I use StringValue below.


http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@40
PS14, Line 40: differentiate be
> Nit: differentiate.
Done


http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@58
PS14, Line 58: }
> Is this a column name? An index? An ID?
Done


http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@59
PS14, Line 59:
> Is this value encoded? Decoded?
Done


http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@68
PS14, Line 68:     int32 seed = 3;
> Column names? Indexes? IDs?
Done


http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@73
PS14, Line 73:     RangePartitionMetadataPB rangePartitions = 2;
> Same.
Done


http://gerrit.cloudera.org:8080/#/c/10375/14/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/10375/14/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@66
PS14, Line 66: prevents
> Nit: prevents
Done


http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
File 
java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala:

http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@41
PS12, Line 41:
> Still to do?
Yeah, because I am not sure what our final limitations are going to be and 
right now that isn't a public "concept" I will put a todo comment.


http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala:

http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@38
PS12, Line 38: sc
> What I meant was, the KuduContext argument to this function is named 'kuduC
Oh, that's actually a pain, because it collides with a variable in the RDD 
class we are extending.

I will note, ideally we would just use the context included in kuduContext, but 
it has serialization issues, so we can't.


http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@87
PS12, Line 87: 
> Nit: indentation.
Done


http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@87
PS12, Line 87:
> Still to do.
Done


http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@91
PS12, Line 91:
> Still to do.
Done


http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@91
PS12, Line 91:
> Not really understanding the purpose of this wrapper. Doc?
Done


http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@97
PS12, Line 97:   * that takes the job partitions and task context and expects 
an Iterator[Row] in return.
> Still to do.
This matches the KuduRDD in SparkContext. We may eventually merge some 
functionality back in so I would like to leave it the same for ease of 
comparison.


http://gerrit.cloudera.org:8080/#/c/10375/14/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/10375/14/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@43
PS14, Line 43: ins't
> Nit: isn't
Done


http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File 
java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:

http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@47
PS12, Line 47:
> OK, can you reference that constant here then?
I want the test to break if the constant changes.


http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-client/src/main/java/org/apache/kudu/Type.java
File java/kudu-client/src/main/java/org/apache/kudu/Type.java:

http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-client/src/main/java/org/apache/kudu/Type.java@194
PS14, Line 194:
> Nit: should add a space before this.
Done



--
To view, visit http://gerrit.cloudera.org:8080/10375
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 16
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
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Comment-Date: Wed, 13 Jun 2018 19:59:36 +0000
Gerrit-HasComments: Yes

Reply via email to