You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2018/05/11 18:41:00 UTC
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10375
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup implimentation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
I will add a Restore job next to test a round trip export
and import.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/TestImportExportFiles.csv
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/pom.xml
M java/settings.gradle
13 files changed, 688 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/1
--
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: newchange
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 18:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/main/protobuf/backup.proto@65
PS18, Line 65: The number of values must be <= the number of columns
The number of values equals the number of columns in the range partition key
http://gerrit.cloudera.org:8080/#/c/10375/18/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/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@41
PS18, Line 41:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@41
PS18, Line 41:
extra space
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@113
PS18, Line 113: // TODO: Move to a PartitionSchema equals/equivalent method
Missing punctuation here and in other TODOs in this file
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@126
PS18, Line 126: Has
hash
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@281
PS18, Line 281: row
does this do anything here?
--
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: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 19 Jun 2018 21:49:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#20).
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Kudu Backup/Restore Spark Jobs
Adds a rough base implementation of Kudu backup and restore
Spark jobs. There are many todos indicating gaps and more testing
and details to be be finished. However, these base jobs work and are
in a functional state that can be committed and iterated on as we
build up and improve our backup functionality.
These jobs, as annotated, should be considered private, unstable,
and experimental.
The backup job can output one to many tables data to any spark
compatible path in any spark compatible format, the defaults being
HDFS and Parquet. Each table’s data is written in a subdirectory of
the provided path. The subdirectory’s name is the url encoded table
name. Additionally in each table’s directory a json metadata file is
output with the metadata needed to recreate the table that was
exported when restoring.
The restore job can read the data and metadata generated and create
“restore” tables with a matching schema and reload the data.
The job arguments are a work in progress and will likely be enhanced
and simplified as we find what is useful and what isn’t through
performance and functional testing. More documentation will be
generated when the jobs are ready for general use.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,594 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/20
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 20
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#13).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
Adds a rough base implementation of Kudu backup and restore Spark jobs. There are many todo’s indicating gaps and more testing and details to be be finished. However, these base jobs work and are in a functional state that can be committed and iterated on as we build up and improve our backup functionality.
These jobs, as annotated, should be considered private, unstable, and experimental.
The backup job can output tables data to any spark compatible path in any spark compatible format. The defaults being HDFS and Parquet. Additionally in the each tables directory a json metadata file is output with the metadata needed to recreate the table that was exported when restoring.
The restore job can read the data and metadata generated. Create “restore” tables with a matching schema and reload the data.
The job arguments are a work in progress and will likely be enhanced and simplified as we find what is useful and what isn’t through performance and functional testing. More documentation will be generated when the jobs are ready for general use.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,555 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/13
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 13
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 17:
(25 comments)
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@32
PS17, Line 32:
> nit: use 2-space indentation in this file for consistency with other .proto
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@46
PS17, Line 46: default
> let's name this default_value since DEFAULT is a keyword in protobuf 2 and
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@52
PS17, Line 52: readible
> readable
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@63
PS17, Line 63: endoded
> encoded
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@65
PS17, Line 65: repeated ColumnValueMetadataPB lower_bounds = 1;
: repeated ColumnValueMetadataPB upper_bounds = 2;
> Doc why these are repeated and what invariants we have (number of component
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@98
PS17, Line 98: // The starting point of a backup. A UNIX timestamp in milliseconds since the epoch.
: int64 from_ms = 1;
: // The end point of a backup. A UNIX timestamp in milliseconds since the epoch.
: int64 to_ms = 2;
> What is the purpose of storing these?
This isn't the start time and end time of the job. It the time used to dictate the snapshot scan. There is a comment where this is used talking about how from_ms is always 0 until we introduce incremental backups.
We store these values so that we can know where a backup left off and properly set the time for a snapshot scan in follow up incremental jobs.
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@103
PS17, Line 103: name
> Can we name this table_name?
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/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/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@50
PS17, Line 50: TODO: Take parameter for the SaveMode
> nit: Add a period or punctuation to the end of all your comments per the C+
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@66
PS17, Line 66: false
> small suggestion: /* overwrite= */ false
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/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/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@37
PS17, Line 37: defaultFormat
> nit: Shouldn't these constants be in UpperCamelCase per https://docs.scala-
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@42
PS17, Line 42: // TODO: clean up usage output
> nit: punctuation in this file too
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@55
PS17, Line 55: timestampMs
> should we specify this as microseconds since that's the resolution we can a
Anything more precise than milliseconds seamed over the top for command line interaction. I suspect most users would just be converting from seconds or milliseconds.
http://gerrit.cloudera.org:8080/#/c/10375/17/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/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@45
PS17, Line 45: // Set a hybrid time for the scan to ensure application consistency
> nit: comment punctuation here and elsewhere
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@46
PS17, Line 46: options.timestampMs
> this is an optional parameter, we should either handle the non-specified ca
It's defaulted to System.currentTimeMillis() in KuduBackupOptions.
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@57
PS17, Line 57: .snapshotTimestampRaw(hybridTime)
> Do we store this? We need to record the chosen timestamp somewhere in order
We store this in the metadata in the to_ms field.
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@78
PS17, Line 78: delta
> I think the terminology we have been using is "incremental", because deltas
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@97
PS17, Line 97: and expects an Iterator[Row] in return
> and expects to return an Iterator[Row]
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@106
PS17, Line 106:
> How about:
Adar commented something similar. This is the exact same logic used in KuduRDD and I wanted to keep them the same.
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@107
PS17, Line 107: if (TaskContext.get().isInterrupted())
> Why is this check inside the loop? Can we move this to the bottom of the ha
Honestly I am not sure. This is code from KuduRDD.
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@108
PS17, Line 108: Kudu task interrupted
> how about: "KuduBackup spark task interrupted", or something similar / more
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@115
PS17, Line 115: TODO
> here and elsewhere: when you write TODO put TODO(grant) or TODO(KUDU-1234)
I don't agree with TODO(name) as no person should own a change/code.
I agree with adding a jira when appropriate, but given this is a work in progress module related to backup I think it is just noise to mark all the TODOs with the same jira.
We also do follow that format very often in the codebase looking at other TODOs.
http://gerrit.cloudera.org:8080/#/c/10375/17/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/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@43
PS17, Line 43: process
> s/process/processed/
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@43
PS17, Line 43: ins't
> typo: isn't -- also add punctuation and TODO(...) to comments in this file
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala:
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@69
PS17, Line 69: opt[String]("format")
> Why wouldn't this be stored in the metadata?
Done
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala:
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala@99
PS17, Line 99: Partitions
> nit: s/Partitions/Partition/
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: 17
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 19 Jun 2018 19:15:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#4).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup/restore implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/TestImportExportFiles.csv
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/pom.xml
M java/settings.gradle
16 files changed, 897 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/4
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/protobuf/backup.proto@36
PS10, Line 36: message ColumnMetadataPB {
> +1 to what adar said. I am writing some generic tests that will iterate ove
Hrm, I think worst case scenario we miss some metadata but that seems less serious than missing actual data. Seems reasonable.
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala:
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@37
PS10, Line 37: opt[Boolean]("createTables")
> Is that different than optional with a default value? Currently this defaul
Ah ok, I had missed that. Thanks!
--
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: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 01 Jun 2018 20:59:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#11).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup/restore implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
17 files changed, 1,445 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/11
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 5:
I addressed some smaller TODOs and added hash partition support. I added initial range partition support but need to work on getting the split values included in the metadata.
--
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: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 14 May 2018 17:50:19 +0000
Gerrit-HasComments: No
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#19).
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Kudu Backup/Restore Spark Jobs
Adds a rough base implementation of Kudu backup and restore
Spark jobs. There are many todos indicating gaps and more testing
and details to be be finished. However, these base jobs work and are
in a functional state that can be committed and iterated on as we
build up and improve our backup functionality.
These jobs, as annotated, should be considered private, unstable,
and experimental.
The backup job can output one to many tables data to any spark
compatible path in any spark compatible format, the defaults being
HDFS and Parquet. Each table’s data is written in a subdirectory of
the provided path. The subdirectory’s name is the url encoded table
name. Additionally in each tables directory a json metadata file is
output with the metadata needed to recreate the table that was
exported when restoring.
The restore job can read the data and metadata generated and create
“restore” tables with a matching schema and reload the data.
The job arguments are a work in progress and will likely be enhanced
and simplified as we find what is useful and what isn’t through
performance and functional testing. More documentation will be
generated when the jobs are ready for general use.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,594 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/19
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 19
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 10:
(2 comments)
Did another high level review. Looking good. Just a couple questions.
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/protobuf/backup.proto@36
PS10, Line 36: message ColumnMetadataPB {
Since we're not using the internal PB, any ideas on how we can guarantee that we don't forget to add a field to the backup protobuf when we add a new field to the application PB?
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala:
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@37
PS10, Line 37: opt[Boolean]("createTables")
nit: maybe this should be required with a fallback (default value)?
--
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: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 31 May 2018 22:38:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#15).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
Adds a rough base implementation of Kudu backup and restore
Spark jobs. There are many todos indicating gaps and more testing
and details to be be finished. However, these base jobs work and are
in a functional state that can be committed and iterated on as we
build up and improve our backup functionality.
These jobs, as annotated, should be considered private, unstable,
and experimental.
The backup job can output one to many tables data to any spark
compatible path in any spark compatible format, the defaults being
HDFS and Parquet. Each table’s data is written in a subdirectory of
the provided path. The subdirectory’s name is the url encoded table
name. Additionally in the each tables directory a json metadata file is
output with the metadata needed to recreate the table that was
exported when restoring.
The restore job can read the data and metadata generated and create
“restore” tables with a matching schema and reload the data.
The job arguments are a work in progress and will likely be enhanced
and simplified as we find what is useful and what isn’t through
performance and functional testing. More documentation will be
generated when the jobs are ready for general use.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,568 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/15
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#3).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup/restore implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/TestImportExportFiles.csv
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/pom.xml
M java/settings.gradle
16 files changed, 891 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/3
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 14:
(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 Spark jobs. There are many todo’s indicating gaps and more testing and details to be be finished. However, these base jobs work and are in a functional state that can be committed and iterated on as we build up and improve our backup functionality.
Nit: please reformat the commit message so it adheres to our style guide's 80 chars per line.
http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG@9
PS14, Line 9: todo’s
Nit: todos
http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG@13
PS14, Line 13: each tables
In the "per table" directory, perhaps? It's a directory per table, right?
http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG@13
PS14, Line 13: . T
Nit: as written, you should combine these two sentences: "in any spark compatible format, the defaults being HDFS and Parquet"
http://gerrit.cloudera.org:8080/#/c/10375/14//COMMIT_MSG@15
PS14, Line 15: The restore job can read the data and metadata generated. Create “restore” tables with a matching schema and reload the data.
The second sentence is a fragment; consider joining it to the first: "...metadata generated, and create "restore" tables..."
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:
PS12:
> Do you have a suggestion what to include so that the doc enhances the descr
The docs should explain to what logical Kudu concept each field is mapped, and how they all work together.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/protobuf/backup.proto@28
PS12, Line 28:
> Done
Can you move TableMetadataPB to the bottom?
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?
http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@40
PS14, Line 40: differententiate
Nit: differentiate.
http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@58
PS14, Line 58: string column = 1;
Is this a column name? An index? An ID?
http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@59
PS14, Line 59: string value = 2;
Is this value encoded? Decoded?
http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@68
PS14, Line 68: repeated string columns = 1;
Column names? Indexes? IDs?
http://gerrit.cloudera.org:8080/#/c/10375/14/java/kudu-backup/src/main/protobuf/backup.proto@73
PS14, Line 73: repeated string columns = 1;
Same.
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: prevent's
Nit: prevents
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:
> Should explain what this means w.r.t. the ancient history watermark.
Still to do?
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
> KuduContext calls it `sc` too.
What I meant was, the KuduContext argument to this function is named 'kuduContext', so perhaps the SparkContext variable should be named 'sparkContext' for symmetry.
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.
Still to do.
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?
Still to do.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@97
PS12, Line 97: override def hasNext: Boolean = {
> Nit: switch this around so the overall condition is more clear.
Still to do.
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
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala:
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@28
PS12, Line 28: kuduMasterAddresses: String = InetAddress.getLocalHost.getCanonicalHostName,
> How might this be formatted? Could you provide more guidance?
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:
> It is a constant in KuduRestoreOptions.defaultTableSuffix
OK, can you reference that constant here then?
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.
In the below condition too.
--
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: 14
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 13 Jun 2018 18:52:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 19:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10375/19//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10375/19//COMMIT_MSG@22
PS19, Line 22: tables
> Nit: table's
Done
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto@56
PS19, Line 56: encoded human-readable
> Aren't these contradictory? I'd expect an encoded value to be human unreada
I mean't encoded in that it's been encoded to a human-readable string, but I can remove the word to avoid confusion.
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto@109
PS19, Line 109: // The number of replicas this table has.
> Nit: technically, it's the number of replicas of each tablet in the table.
I will update the comment.
I guess num_replicas isn't really a great name for the field either then? It's taken directly from CreateTableOptions.setNumReplicas(...) and KuduTable.getNumReplicas(...).
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto@111
PS19, Line 111: // The metadata for the tables columns.
> Nit: table's
Done
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto@113
PS19, Line 113: tables
> Nit: table's
Done
http://gerrit.cloudera.org:8080/#/c/10375/19/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/19/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@37
PS19, Line 37: val DefaultFormat: String = "parquet"
: val DefaultScanBatchSize: Int = 1024*1024*20 // 20 MiB
: val DefaultScanRequestTimeout: Long = AsyncKuduClient.DEFAULT_OPERATION_TIMEOUT_MS // 30 seconds
: val DefaultScanPrefetching: Boolean = false // TODO: Add a test per KUDU-1260 and enable by default?
> I brought this up earlier (they were camelCase) and UpperCamelCase is corre
Like mike said this matches Scala's standard.
--
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: 19
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 25 Jun 2018 14:30:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#9).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup/restore implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KeyEncoder.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,299 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/9
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 8:
(1 comment)
I haven't reviewed the code at all but I wanted to comment on one of Mike's suggestions.
http://gerrit.cloudera.org:8080/#/c/10375/8/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/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@65
PS8, Line 65: Serialization.write(metadata, out)
> Rather than use Spark serialization based on POJO (POSO?) definitions for m
+1 to defining the schema in protobuf.
However, I don't think we should use SchemaPB/PartitionSchemaPB directly. I think there's value in this code being as "third party" as possible. Meaning, as much as it makes sense, it shouldn't leverage Kudu internals, and it should use Kudu public APIs. That separation means this could serve as a reference implementation and can be ported to other execution frameworks with a minimum of fuss. Plus actually using internal Kudu protobufs would probably require poking holes in our shading scheme.
Along the same lines, I think a container format is unnecessary, and I certainly wouldn't want us to port PBC to Java. I think we can get by with a simple pb to JSON conversion; protobuf provides utilities that do this.
--
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: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 18 May 2018 03:59:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#17).
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Kudu Backup/Restore Spark Jobs
Adds a rough base implementation of Kudu backup and restore
Spark jobs. There are many todos indicating gaps and more testing
and details to be be finished. However, these base jobs work and are
in a functional state that can be committed and iterated on as we
build up and improve our backup functionality.
These jobs, as annotated, should be considered private, unstable,
and experimental.
The backup job can output one to many tables data to any spark
compatible path in any spark compatible format, the defaults being
HDFS and Parquet. Each table’s data is written in a subdirectory of
the provided path. The subdirectory’s name is the url encoded table
name. Additionally in each tables directory a json metadata file is
output with the metadata needed to recreate the table that was
exported when restoring.
The restore job can read the data and metadata generated and create
“restore” tables with a matching schema and reload the data.
The job arguments are a work in progress and will likely be enhanced
and simplified as we find what is useful and what isn’t through
performance and functional testing. More documentation will be
generated when the jobs are ready for general use.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,595 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/17
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 17
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 19: Verified+1
--
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: 19
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 20 Jun 2018 14:25:27 +0000
Gerrit-HasComments: No
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 19
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Kudu Backup/Restore Spark Jobs
Adds a rough base implementation of Kudu backup and restore
Spark jobs. There are many todos indicating gaps and more testing
and details to be be finished. However, these base jobs work and are
in a functional state that can be committed and iterated on as we
build up and improve our backup functionality.
These jobs, as annotated, should be considered private, unstable,
and experimental.
The backup job can output one to many tables data to any spark
compatible path in any spark compatible format, the defaults being
HDFS and Parquet. Each table’s data is written in a subdirectory of
the provided path. The subdirectory’s name is the url encoded table
name. Additionally in each table’s directory a json metadata file is
output with the metadata needed to recreate the table that was
exported when restoring.
The restore job can read the data and metadata generated and create
“restore” tables with a matching schema and reload the data.
The job arguments are a work in progress and will likely be enhanced
and simplified as we find what is useful and what isn’t through
performance and functional testing. More documentation will be
generated when the jobs are ready for general use.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Reviewed-on: http://gerrit.cloudera.org:8080/10375
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,594 insertions(+), 7 deletions(-)
Approvals:
Kudu Jenkins: Verified
Mike Percy: Looks good to me, approved
--
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: merged
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 21
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#10).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup/restore implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
17 files changed, 1,251 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/10
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 16:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10375/16//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10375/16//COMMIT_MSG@22
PS16, Line 22: Additionally in the each tables directory
> This still doesn't read correctly; what is "the each tables directory"?
Done
http://gerrit.cloudera.org:8080/#/c/10375/16/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
PS16:
> Just remembered that, stylistically, we use underscore_delimited_names for
Done
http://gerrit.cloudera.org:8080/#/c/10375/16/java/kudu-backup/src/main/protobuf/backup.proto@77
PS16, Line 77: int64 fromMs = 1;
: int64 toMs = 2;
> What do these mean? Are they quantities of msec from the UNIX epoch? Please
Done
http://gerrit.cloudera.org:8080/#/c/10375/16/java/kudu-backup/src/main/protobuf/backup.proto@80
PS16, Line 80: int32 replicas = 4;
> Nit: would prefer num_replicas.
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 <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 18 Jun 2018 17:18:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 16:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10375/16//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10375/16//COMMIT_MSG@22
PS16, Line 22: Additionally in the each tables directory
This still doesn't read correctly; what is "the each tables directory"?
http://gerrit.cloudera.org:8080/#/c/10375/16/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
PS16:
Just remembered that, stylistically, we use underscore_delimited_names for proto fields, not camelCase. See src/kudu/common/common.proto for an example.
http://gerrit.cloudera.org:8080/#/c/10375/16/java/kudu-backup/src/main/protobuf/backup.proto@77
PS16, Line 77: int64 fromMs = 1;
: int64 toMs = 2;
What do these mean? Are they quantities of msec from the UNIX epoch? Please doc this, and please doc the rest of the messages/fields as I suggested earlier.
http://gerrit.cloudera.org:8080/#/c/10375/16/java/kudu-backup/src/main/protobuf/backup.proto@80
PS16, Line 80: int32 replicas = 4;
Nit: would prefer num_replicas.
--
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 <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 13 Jun 2018 20:33:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 20: Code-Review+2
--
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: 20
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 25 Jun 2018 19:31:48 +0000
Gerrit-HasComments: No
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 8:
(13 comments)
I suck at Scala but I did a first pass on this. Overall it looks very good. Let me know if you want specific feedback on anything. Since this is a large patch already, I think it would be valuable to do some cleanup where needed and commit a baseline before continuing to add more features to this patch. Also I resisted the urge to point out formatting nits but we can do a pass like that before commit a bit later.
http://gerrit.cloudera.org:8080/#/c/10375/8/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/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@33
PS8, Line 33: object KuduBackup {
Can we mark this as experimental so we can commit some of this code even before it's fully baked?
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@55
PS8, Line 55: writeTableMetadata(tableMetadata, tablePath, session)
We don't have to solve this immediately, and it has been mentioned before in design discussions, but it at least deserves a TODO here: what happens if there are new columns/partitions created or existing columns/partitions dropped while we are doing the backup? It seems that we are doing the backup and then collecting the table metadata when we are done and so right now there is nothing guaranteeing that the stored data on disk exactly matches up with the schema info written in the metadata file. At some point we considered prohibiting DDL ops while doing a backup to avoid that race, I'm sure there are other things we can do as well.
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@65
PS8, Line 65: Serialization.write(metadata, out)
Rather than use Spark serialization based on POJO (POSO?) definitions for metadata files, I would like to see us use protobufs for the metadata so that evolving the fields is straightforward. I think it would make sense to define the serialization format in a .proto file and for the column schema and partition schema structs, hopefully we can directly reference SchemaPB and PartitionSchemaPB from src/kudu/common/common.proto
One caveat is figuring out the right container format to use for the serialized protobuf. One option is the PB container file format we use elsewhere in Kudu, the main drawback being that we would have to port the reader / writer code from C++ to Java, although that probably isn't very difficult. Another option could be to store it as Avro, and use a protobuf-to-Avro file converter library, of which there are a couple of implementations out there.
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@79
PS8, Line 79: move to a utility class
yeah, +1
http://gerrit.cloudera.org:8080/#/c/10375/8/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/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@30
PS8, Line 30: Address
nit: maybe we should name this "addresses"
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@37
PS8, Line 37: timestamp
nit: UNIX timestamp in milliseconds since the epoch
http://gerrit.cloudera.org:8080/#/c/10375/8/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/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@33
PS8, Line 33: @transient val table
how does this get reconstructed after deserialization?
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@38
PS8, Line 38: smaller scan tokens
you probably saw this but someone started working on this at https://gerrit.cloudera.org/c/10406/
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@65
PS8, Line 65: Array(leaderLocation)
Hmm, we may want to simply use token.getTablet.getReplicas because the leader might change at any time
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@69
PS8, Line 69: Do we need a custom spark partitioner
good question, I don't know the answer to this either right now but worth looking into
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@89
PS8, Line 89: efficiency
what kind of efficiency do you mean?
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@124
PS8, Line 124: Move to a utility class?
+1
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala:
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@52
PS8, Line 52: @transient
Pardon my Scala-newbness, but is there some code somewhere to re-initialize the 'sc' class field when an instance of KuduContext is deserialized?
--
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: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 18 May 2018 03:38:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/protobuf/backup.proto@36
PS10, Line 36: message ColumnMetadataPB {
> This PB doesn't need to be synchronized with the internal ColumnSchemaPB; i
+1 to what adar said. I am writing some generic tests that will iterate over "all" options that would hopefully exposing a missing feature, unsupported type, etc.
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala:
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@37
PS10, Line 37: opt[Boolean]("createTables")
> nit: maybe this should be required with a fallback (default value)?
Is that different than optional with a default value? Currently this defaults to false because the KuduRestoreOptions class defaults it to `defaultCreateTables`.
--
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: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 01 Jun 2018 15:26:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 19:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10375/19/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/19/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@37
PS19, Line 37: val DefaultFormat: String = "parquet"
: val DefaultScanBatchSize: Int = 1024*1024*20 // 20 MiB
: val DefaultScanRequestTimeout: Long = AsyncKuduClient.DEFAULT_OPERATION_TIMEOUT_MS // 30 seconds
: val DefaultScanPrefetching: Boolean = false // TODO: Add a test per KUDU-1260 and enable by default?
> If these are equivalent to "public static final ..." in Java, should they b
I brought this up earlier (they were camelCase) and UpperCamelCase is correct for constants according to the Scala Style Guide @ https://docs.scala-lang.org/style/naming-conventions.html#constants-values-variable-and-methods
--
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: 19
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Jun 2018 21:56:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#16).
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Kudu Backup/Restore Spark Jobs
Adds a rough base implementation of Kudu backup and restore
Spark jobs. There are many todos indicating gaps and more testing
and details to be be finished. However, these base jobs work and are
in a functional state that can be committed and iterated on as we
build up and improve our backup functionality.
These jobs, as annotated, should be considered private, unstable,
and experimental.
The backup job can output one to many tables data to any spark
compatible path in any spark compatible format, the defaults being
HDFS and Parquet. Each table’s data is written in a subdirectory of
the provided path. The subdirectory’s name is the url encoded table
name. Additionally in the each tables directory a json metadata file is
output with the metadata needed to recreate the table that was
exported when restoring.
The restore job can read the data and metadata generated and create
“restore” tables with a matching schema and reload the data.
The job arguments are a work in progress and will likely be enhanced
and simplified as we find what is useful and what isn’t through
performance and functional testing. More documentation will be
generated when the jobs are ready for general use.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,568 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/16
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 16
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 19:
(7 comments)
Looks good, just nits.
http://gerrit.cloudera.org:8080/#/c/10375/19//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10375/19//COMMIT_MSG@22
PS19, Line 22: tables
Nit: table's
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto@56
PS19, Line 56: encoded human-readable
Aren't these contradictory? I'd expect an encoded value to be human unreadable, vs. a value that had been decoded using the table's schema. BTW here "encoded" refers to Kudu-based encoding, not something like UTF-8.
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto@109
PS19, Line 109: // The number of replicas this table has.
Nit: technically, it's the number of replicas of each tablet in the table. Or you can call it the "replication factor" of the table.
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto@111
PS19, Line 111: // The metadata for the tables columns.
Nit: table's
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/protobuf/backup.proto@113
PS19, Line 113: tables
Nit: table's
http://gerrit.cloudera.org:8080/#/c/10375/19/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/19/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@37
PS19, Line 37: val DefaultFormat: String = "parquet"
: val DefaultScanBatchSize: Int = 1024*1024*20 // 20 MiB
: val DefaultScanRequestTimeout: Long = AsyncKuduClient.DEFAULT_OPERATION_TIMEOUT_MS // 30 seconds
: val DefaultScanPrefetching: Boolean = false // TODO: Add a test per KUDU-1260 and enable by default?
If these are equivalent to "public static final ..." in Java, should they be UPPER_CASE too?
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala:
http://gerrit.cloudera.org:8080/#/c/10375/19/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@34
PS19, Line 34: val DefaultTableSuffix: String = "-restore"
: val DefaultCreateTables: Boolean = true
Same question.
--
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: 19
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 20 Jun 2018 21:16:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 17:
(25 comments)
Overall looks good, all minor feedback.
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@32
PS17, Line 32:
nit: use 2-space indentation in this file for consistency with other .proto files in the code base
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@46
PS17, Line 46: default
let's name this default_value since DEFAULT is a keyword in protobuf 2 and it might be a little confusing
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@52
PS17, Line 52: readible
readable
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@63
PS17, Line 63: endoded
encoded
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@65
PS17, Line 65: repeated ColumnValueMetadataPB lower_bounds = 1;
: repeated ColumnValueMetadataPB upper_bounds = 2;
Doc why these are repeated and what invariants we have (number of components in lower_bounds and upper_bounds equals number of columns involved in the range partition)
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@98
PS17, Line 98: // The starting point of a backup. A UNIX timestamp in milliseconds since the epoch.
: int64 from_ms = 1;
: // The end point of a backup. A UNIX timestamp in milliseconds since the epoch.
: int64 to_ms = 2;
What is the purpose of storing these?
Maybe this would be better expressed as startTime and duration?
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/protobuf/backup.proto@103
PS17, Line 103: name
Can we name this table_name?
http://gerrit.cloudera.org:8080/#/c/10375/17/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/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@50
PS17, Line 50: TODO: Take parameter for the SaveMode
nit: Add a period or punctuation to the end of all your comments per the C++ style guide @ https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar (I know this is not C++) and for consistency with the rest of the Kudu code base.
Here and in the rest of this patch.
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@66
PS17, Line 66: false
small suggestion: /* overwrite= */ false
instead of the trailing comment
http://gerrit.cloudera.org:8080/#/c/10375/17/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/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@37
PS17, Line 37: defaultFormat
nit: Shouldn't these constants be in UpperCamelCase per https://docs.scala-lang.org/style/naming-conventions.html#constants-values-variable-and-methods ?
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@42
PS17, Line 42: // TODO: clean up usage output
nit: punctuation in this file too
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@55
PS17, Line 55: timestampMs
should we specify this as microseconds since that's the resolution we can accept internally anyway?
http://gerrit.cloudera.org:8080/#/c/10375/17/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/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@45
PS17, Line 45: // Set a hybrid time for the scan to ensure application consistency
nit: comment punctuation here and elsewhere
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@46
PS17, Line 46: options.timestampMs
this is an optional parameter, we should either handle the non-specified case or add a TODO for that
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@57
PS17, Line 57: .snapshotTimestampRaw(hybridTime)
Do we store this? We need to record the chosen timestamp somewhere in order to support incremental backups
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@78
PS17, Line 78: delta
I think the terminology we have been using is "incremental", because deltas already are a thing in Kudu
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@97
PS17, Line 97: and expects an Iterator[Row] in return
and expects to return an Iterator[Row]
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@106
PS17, Line 106:
How about:
while (scanner.hasMoreRows && (currentIterator == null || !currentIterator.hasNext))
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@107
PS17, Line 107: if (TaskContext.get().isInterrupted())
Why is this check inside the loop? Can we move this to the bottom of the hasNext method?
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@108
PS17, Line 108: Kudu task interrupted
how about: "KuduBackup spark task interrupted", or something similar / more specific
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@115
PS17, Line 115: TODO
here and elsewhere: when you write TODO put TODO(grant) or TODO(KUDU-1234) or something like that so it's easier to keep track of the action item related to the TODO, per https://google.github.io/styleguide/cppguide.html#TODO_Comments and consistency with the rest of the Kudu code base.
http://gerrit.cloudera.org:8080/#/c/10375/17/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/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@43
PS17, Line 43: ins't
typo: isn't -- also add punctuation and TODO(...) to comments in this file too
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@43
PS17, Line 43: process
s/process/processed/
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala:
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@69
PS17, Line 69: opt[String]("format")
Why wouldn't this be stored in the metadata?
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala:
http://gerrit.cloudera.org:8080/#/c/10375/17/java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala@99
PS17, Line 99: Partitions
nit: s/Partitions/Partition/
--
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: 17
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 19 Jun 2018 01:31:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/10/java/kudu-backup/src/main/protobuf/backup.proto@36
PS10, Line 36: message ColumnMetadataPB {
> Since we're not using the internal PB, any ideas on how we can guarantee th
This PB doesn't need to be synchronized with the internal ColumnSchemaPB; it only needs to be synchronized with whatever client APIs are used to backup tables (Grant's new partition fetching APIs) and restore tables (Create Table). And since those are client APIs and not internal PBs, they're more likely to change in backward-compatible ways, so the coupling to this PB could remain loose.
--
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: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 31 May 2018 23:06:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 13:
(34 comments)
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/build.gradle
File java/kudu-backup/build.gradle:
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/build.gradle@26
PS12, Line 26: compile (libs.protobufJavaUtil) {
: // Make sure wrong Guava version is not pulled in.
: exclude group: "com.google.guava", module: "guava"
: }
: compile (libs.scopt) {
:
> Doc why we need these exclusions?
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/pom.xml
File java/kudu-backup/pom.xml:
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/pom.xml@211
PS12, Line 211: <plugin>
> Need to specify ${maven-surefire-plugin.version} for the version?
We define that in plugin-management on the root pom. Not sure why we use plugin-management sometimes but not others. Also not sure why we don't use dependency-management.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/pom.xml@221
PS12, Line 221: </plugins>
> Do we actually need to replica this here?
I assume this is what makes eclipse work. I added it for that. Hopefully maven is gone soon. :D
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:
PS12:
> Please add a short doc to each message definition.
Do you have a suggestion what to include so that the doc enhances the descriptive names and the overall summary at the top?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/protobuf/backup.proto@19
PS12, Line 19: // Though these are similar to the kudu common protobufs,
: // they are specific to the kudu backup application and
: // should be kept seperate.
> FWIW, I think the stronger explanation is that these are strictly "applicat
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/protobuf/backup.proto@28
PS12, Line 28:
> Nit: stylistically, we tend to order our PB definitions such that "leaf" me
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/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/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@50
PS12, Line 50: // TODO: Take parameter for the SaveMode
> What effect does this have? Parallelism of the backup process?
I was thinking this would be a way to logically define which files go with the partitions. However, after further discussion adding more information in the metadata file or even using the metadata in parquet itself is likely a good approach when that is needed.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@65
PS12, Line 65: val json = JsonFormat.printer().print(metadata)
> Doc what does 'false' means here.
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@74
PS12, Line 74:
> Can we add some useful text to this exception?
Scopt (the argument parser library) will print an error message to the CLI in the case we couldn't parse the options. That said I can still add generic text to the error message.
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:
PS12:
> License.
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@31
PS12, Line 31: format: String = KuduBackupOptions.defaultFormat,
> Nit: some of these are terminated with a period and some aren't. Fix?
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@36
PS12, Line 36: object KuduBackupOptions {
> Nit: "Comma-separated addresses of Kudu masters"
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@46
PS12, Line 46: .action((v, o) => o.copy(path = v))
> What are valid values for this?
I have a todo to limit the "accepted" formats in the code where this argument is used. I expect we will provide an enum of valid values or eliminate this once we settle on a format. For now this allows experimenting with any format the spark supports.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@49
PS12, Line 49:
> Is this something really worth customizing?
We may eliminate this, but it exists so we can experiment during our performance testing.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@61
PS12, Line 61: .action((v, o) => o.copy(format = v))
> Do we really want to expose this?
We may eliminate this, but it exists so we can experiment during our performance testing.
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
> Nit: maybe sparkContext for symmetry with kuduContext?
KuduContext calls it `sc` too.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@49
PS12, Line 49: // Create the scan tokens for each partition
> Doc why the scan is fault tolerant and why we have to use LEADER_ONLY.
I have a todo below to provide better support for picking replicas. For now this is just for simplicity.
http://gerrit.cloudera.org:8080/#/c/10375/12/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/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@61
PS12, Line 61: val writeOptions = new KuduWriteOptions(ignoreDuplicateRowErrors = false, ignoreNull = false)
> Doc what the two 'false' args mean.
Since KuduWriteOptions is a scala class I can use named parameters to make this more clear. I will do that.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@87
PS12, Line 87: .getOrElse(throw new IllegalArgumentException("could not parse the arguments"))
> Add useful text.
See my comment on KuduBackup.scala
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala:
PS12:
> License.
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@18
PS12, Line 18:
> Much of the feedback I left on KuduBackupOptions applies here too.
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@38
PS12, Line 38:
> Only relevant when createTables is true, right?
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@43
PS12, Line 43: .action((v, o) => o.copy(path = v))
> Nit: "true to create tables during restore, false if they already exist"
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@48
PS12, Line 48: .action((v, o) => o.copy(kuduMasterAddresses = v))
> Not really clear what the "overriding" behavior means; there's not enough c
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:
> Can you make this a constant somewhere so that if we decide to change the d
It is a constant in KuduRestoreOptions.defaultTableSuffix
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@58
PS12, Line 58:
: test("Random Backup and Restore") {
: Random.javaRandomToR
> One thing we found useful is, if you look at SeedRandom() (C++ Kudu test fu
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@79
PS12, Line 79: // TODO: Move to a Schema equals/equivalent method
> This seems like it'd be generally useful outside of BDR. Is there a natural
This could actually be refactored all the way back to the kudu-client tests. I would like to do that to make it more generally useable, but maybe as a follow up? I will leave a todo.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@95
PS12, Line 95: defaultValuesMatch(before.getDefaultValue, after.getDefaultValue) &&
: Objects.equal(before.getDesiredBlockSize, after.getDesiredBlockSize) &&
: Objects.equal(before.getEncoding, after.getEncoding) &&
> Moe this to L138? No need to compute if we're not going to use it. Not sure
I define this upfront given I need it in 2 contexts. I need it for defining the column and the default value. This seamed the most simple approach given it's just randomly generated, needs to match, and it's test code. I am open to suggestions for a better way though.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@101
PS12, Line 101:
> Can this be expressed as a match statement? Seems like it'd be easier to re
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@114
PS12, Line 114: def partitionSchemasMatch(before: PartitionSchema, after: PartitionSchema): Boolean = {
> Move this to L142? No need to compute it if we're not going to use a defaul
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@150
PS12, Line 150: val precision = Random.nextInt(DecimalUtil.MAX_DECIMAL_PRECISION) + 1
> Our tests all use just one tserver? That's disappointing.
Yeah, per the base spark test. We can and should improve this in a separate patch.
http://gerrit.cloudera.org:8080/#/c/10375/12/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/12/java/kudu-client/src/main/java/org/apache/kudu/Type.java@194
PS12, Line 194: for (Type t : values()){
> Nit: for (Type ...
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-client/src/main/java/org/apache/kudu/Type.java@195
PS12, Line 195: if (t.name().equals(name)){
> Nit: if (t.name() ...
Done
http://gerrit.cloudera.org:8080/#/c/10375/12/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/10375/12/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala@117
PS12, Line 117: .setNumReplicas(1)
> Nit: indentation.
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: 13
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 13 Jun 2018 15:53:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 18:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/main/protobuf/backup.proto
File java/kudu-backup/src/main/protobuf/backup.proto:
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/main/protobuf/backup.proto@65
PS18, Line 65: The number of values must be <= the number of columns
> The number of values equals the number of columns in the range partition ke
Done
http://gerrit.cloudera.org:8080/#/c/10375/18/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/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@41
PS18, Line 41:
> extra space
Done
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@41
PS18, Line 41:
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@113
PS18, Line 113: // TODO: Move to a PartitionSchema equals/equivalent method
> Missing punctuation here and in other TODOs in this file
Done
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@126
PS18, Line 126: Has
> hash
Done
http://gerrit.cloudera.org:8080/#/c/10375/18/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@281
PS18, Line 281: row
> does this do anything here?
In Scala there is no explicit return because everything returns a value. This makes the row be the returned value as opposed to the result of `kuduSession.apply(upsert)`
--
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: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 20 Jun 2018 02:46:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#14).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
Adds a rough base implementation of Kudu backup and restore Spark jobs. There are many todo’s indicating gaps and more testing and details to be be finished. However, these base jobs work and are in a functional state that can be committed and iterated on as we build up and improve our backup functionality.
These jobs, as annotated, should be considered private, unstable, and experimental.
The backup job can output tables data to any spark compatible path in any spark compatible format. The defaults being HDFS and Parquet. Additionally in the each tables directory a json metadata file is output with the metadata needed to recreate the table that was exported when restoring.
The restore job can read the data and metadata generated. Create “restore” tables with a matching schema and reload the data.
The job arguments are a work in progress and will likely be enhanced and simplified as we find what is useful and what isn’t through performance and functional testing. More documentation will be generated when the jobs are ready for general use.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,557 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/14
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#18).
Change subject: Kudu Backup/Restore Spark Jobs
......................................................................
Kudu Backup/Restore Spark Jobs
Adds a rough base implementation of Kudu backup and restore
Spark jobs. There are many todos indicating gaps and more testing
and details to be be finished. However, these base jobs work and are
in a functional state that can be committed and iterated on as we
build up and improve our backup functionality.
These jobs, as annotated, should be considered private, unstable,
and experimental.
The backup job can output one to many tables data to any spark
compatible path in any spark compatible format, the defaults being
HDFS and Parquet. Each table’s data is written in a subdirectory of
the provided path. The subdirectory’s name is the url encoded table
name. Additionally in each tables directory a json metadata file is
output with the metadata needed to recreate the table that was
exported when restoring.
The restore job can read the data and metadata generated and create
“restore” tables with a matching schema and reload the data.
The job arguments are a work in progress and will likely be enhanced
and simplified as we find what is useful and what isn’t through
performance and functional testing. More documentation will be
generated when the jobs are ready for general use.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestUtils.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
18 files changed, 1,594 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/18
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#5).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup/restore implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/TestImportExportFiles.csv
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/pom.xml
M java/settings.gradle
16 files changed, 948 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/5
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#7).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup/restore implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/pom.xml
M java/settings.gradle
15 files changed, 1,022 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/7
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#12).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup/restore implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/protobuf/backup.proto
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
M java/settings.gradle
16 files changed, 1,438 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/12
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 12
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 12:
(39 comments)
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/build.gradle
File java/kudu-backup/build.gradle:
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/build.gradle@26
PS12, Line 26: compile (libs.protobufJavaUtil) {
: exclude group: "com.google.guava", module: "guava"
: }
: compile (libs.scopt) {
: exclude group: "org.scala-lang", module: "scala-library"
: }
Doc why we need these exclusions?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/pom.xml
File java/kudu-backup/pom.xml:
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/pom.xml@211
PS12, Line 211: <artifactId>maven-surefire-plugin</artifactId>
Need to specify ${maven-surefire-plugin.version} for the version?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/pom.xml@221
PS12, Line 221: <!-- This big ol' block of nonsense tells the m2e Eclipse plugin what
Do we actually need to replica this here?
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:
PS12:
Please add a short doc to each message definition.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/protobuf/backup.proto@19
PS12, Line 19: // Though these are similar to the kudu common protobufs,
: // they are kept seperate so they can be optimized for
: // the backup process specificially.
FWIW, I think the stronger explanation is that these are strictly "application-level" and thus shouldn't be part of Kudu proper itself.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/protobuf/backup.proto@28
PS12, Line 28:
Nit: stylistically, we tend to order our PB definitions such that "leaf" messages come first, and messages with dependencies on other messages go last.
http://gerrit.cloudera.org:8080/#/c/10375/12/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/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@50
PS12, Line 50: // TODO: Add partitionBy clause to match table partitions in some way?
What effect does this have? Parallelism of the backup process?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@65
PS12, Line 65: val out = fs.create(hPath, false)
Doc what does 'false' means here.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@74
PS12, Line 74: .getOrElse(throw new IllegalArgumentException())
Can we add some useful text to this exception?
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:
PS12:
License.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@31
PS12, Line 31: .text("The root path to output backup data.")
Nit: some of these are terminated with a period and some aren't. Fix?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@36
PS12, Line 36: .text("Comma separated addresses of Kudu master nodes")
Nit: "Comma-separated addresses of Kudu masters"
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@41
PS12, Line 41: .text("A UNIX timestamp in milliseconds since the epoch to execute scans at")
Should explain what this means w.r.t. the ancient history watermark.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@46
PS12, Line 46: .text("The file format to use when writing the data.")
What are valid values for this?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@49
PS12, Line 49: opt[Int]("scanBatchSize")
Is this something really worth customizing?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@61
PS12, Line 61: .text("An experimental flag to enable pre-fetching data")
Do we really want to expose this?
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
Nit: maybe sparkContext for symmetry with kuduContext?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@49
PS12, Line 49: // Create the scan tokens for each partition
Doc why the scan is fault tolerant and why we have to use LEADER_ONLY.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@87
PS12, Line 87: scanToken: Array[Byte],
Nit: indentation.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@91
PS12, Line 91: private class RowIterator(private val scanner: KuduScanner) extends Iterator[Row] {
Not really understanding the purpose of this wrapper. Doc?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@97
PS12, Line 97: (scanner.hasMoreRows && currentIterator == null)) {
Nit: switch this around so the overall condition is more clear.
http://gerrit.cloudera.org:8080/#/c/10375/12/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/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@61
PS12, Line 61: val writeOptions = new KuduWriteOptions(false, false)
Doc what the two 'false' args mean.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@87
PS12, Line 87: .getOrElse(throw new IllegalArgumentException())
Add useful text.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala:
PS12:
License.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@18
PS12, Line 18: object KuduRestoreOptions {
Much of the feedback I left on KuduBackupOptions applies here too.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@28
PS12, Line 28: .text("The root path to the backup data.")
How might this be formatted? Could you provide more guidance?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@38
PS12, Line 38: .text("The suffix to add to the restored table names")
Only relevant when createTables is true, right?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@43
PS12, Line 43: .text("true if you would like to create the target tables, false if they already exist")
Nit: "true to create tables during restore, false if they already exist"
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala@48
PS12, Line 48: .text("The root path to look for table metadata. Useful when overriding metadata of restored tables")
Not really clear what the "overriding" behavior means; there's not enough context to understand.
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: -restore
Can you make this a constant somewhere so that if we decide to change the default suffix, we don't have to update production code AND tests?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@58
PS12, Line 58: val seed = Random.nextLong()
: log.info(s"Setting the random seed to $seed")
: Random.setSeed(seed)
One thing we found useful is, if you look at SeedRandom() (C++ Kudu test function), there's a command line flag for setting the seed to some value. That way if a test fails at a particular seed, it's really easy to repro.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@79
PS12, Line 79: def createRandomTable(): KuduTable = {
This seems like it'd be generally useful outside of BDR. Is there a natural "kudu spark test utils" place into which it could be refactored?
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@95
PS12, Line 95: val precision = Random.nextInt(DecimalUtil.MAX_DECIMAL_PRECISION) + 1
: val scale = Random.nextInt(precision)
: val typeAttributes = DecimalUtil.typeAttributes(precision, scale)
Moe this to L138? No need to compute if we're not going to use it. Not sure how well that's going to work with L124 though.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@101
PS12, Line 101: val encodings: Array[Encoding] =
Can this be expressed as a match statement? Seems like it'd be easier to read.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@114
PS12, Line 114: val defaultValue =
Move this to L142? No need to compute it if we're not going to use a default value.
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@150
PS12, Line 150: val options = new CreateTableOptions().setNumReplicas(1)
Our tests all use just one tserver? That's disappointing.
http://gerrit.cloudera.org:8080/#/c/10375/12/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/12/java/kudu-client/src/main/java/org/apache/kudu/Type.java@194
PS12, Line 194: for(Type t : values()){
Nit: for (Type ...
http://gerrit.cloudera.org:8080/#/c/10375/12/java/kudu-client/src/main/java/org/apache/kudu/Type.java@195
PS12, Line 195: if( t.name().equals(name)){
Nit: if (t.name() ...
http://gerrit.cloudera.org:8080/#/c/10375/12/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/10375/12/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala@117
PS12, Line 117: .setNumReplicas(1)
Nit: indentation.
--
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: 12
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 06 Jun 2018 22:41:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#2).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
I will add a Restore job next to test a round trip export
and import.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/TestImportExportFiles.csv
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/pom.xml
M java/settings.gradle
13 files changed, 688 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/2
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10375/8/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/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@65
PS8, Line 65: Serialization.write(metadata, out)
> +1 to defining the schema in protobuf.
I will move to define this in Protobuf and use generic PB to Json.
http://gerrit.cloudera.org:8080/#/c/10375/8/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/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@30
PS8, Line 30: Address
> nit: maybe we should name this "addresses"
Done
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@37
PS8, Line 37: timestamp
> nit: UNIX timestamp in milliseconds since the epoch
Done
http://gerrit.cloudera.org:8080/#/c/10375/8/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/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@33
PS8, Line 33: @transient val table
> how does this get reconstructed after deserialization?
It doesn't. This is marked as @trasient so that it doesn't get serialized and deserialized. This means it can only be used on the driver. It's only needed on the driver.
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@65
PS8, Line 65: Array(leaderLocation)
> Hmm, we may want to simply use token.getTablet.getReplicas because the lead
This will change when I support backups from the followers (shortly)
http://gerrit.cloudera.org:8080/#/c/10375/8/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@89
PS8, Line 89: efficiency
> what kind of efficiency do you mean?
I mean to avoid decoding and re encoding the rows. We could consider just writing out raw "row" bytes instead of the typed data.
--
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: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 22 May 2018 19:07:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10375
to look at the new patch set (#6).
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
WIP: Kudu Backup/Restore Spark Jobs
This is an extreemly rough backup/restore implementation using
Spark. There are many todo’s indicating major gaps and
a lot of testing and details to be worked out yet.
This is being shared in the interest of feedback and
collaboration, expect major updates and changes.
Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
---
M java/gradle/dependencies.gradle
A java/kudu-backup/build.gradle
A java/kudu-backup/pom.xml
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup/src/test/resources/log4j.properties
A java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-client/src/main/java/org/apache/kudu/Type.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/pom.xml
M java/settings.gradle
15 files changed, 1,014 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/10375/6
--
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: newpatchset
Gerrit-Change-Id: If02183a2f833ffa0225eb7b0a35fc7531109e6f7
Gerrit-Change-Number: 10375
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Kudu Backup/Restore Spark Jobs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 13 Jun 2018 19:59:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: Kudu Backup/Restore Spark Jobs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10375 )
Change subject: WIP: Kudu Backup/Restore Spark Jobs
......................................................................
Patch Set 9:
(1 comment)
Did a high level pass. Only one real suggestion.
http://gerrit.cloudera.org:8080/#/c/10375/9/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/9/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala@84
PS9, Line 84: def main(args: Array[String]): Unit = {
I still think that backup and restore should be part of the same Spark application, though I could be convinced otherwise.
--
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: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 29 May 2018 22:51:43 +0000
Gerrit-HasComments: Yes