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 2019/05/13 14:42:08 UTC
[kudu-CR] [backup] Factor out kudu-backup-tools module
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13320
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
[backup] Factor out kudu-backup-tools module
This patch refactors the kudu-backup module to make
a kudu-backup-tools module. This module will be used
in a future patch to create a more lightweight CLI tool
that shades in all of it’s dependencies.
While refactoring, I also broke out SessionIO into
BackupIO and BackupUtils.
Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
---
M java/gradle/shadow.gradle
A java/kudu-backup-tools/build.gradle
R java/kudu-backup-tools/src/main/protobuf/backup.proto
R java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/BackupGraph.scala
R java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/BackupIO.scala
R java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup-tools/src/test/resources/log4j.properties
R java/kudu-backup-tools/src/test/scala/org/apache/kudu/backup/TestBackupGraph.scala
M java/kudu-backup/build.gradle
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupUtils.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/settings.gradle
15 files changed, 211 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/13320/1
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Mike Percy, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13320
to look at the new patch set (#2).
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
[backup] Factor out kudu-backup-tools module
This patch refactors the kudu-backup module to make
a kudu-backup-tools module. This module will be used
in a future patch to create a more lightweight CLI tool
that shades in all of its dependencies.
While refactoring, I also broke out SessionIO into
BackupIO and BackupUtils.
Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
---
M java/gradle/shadow.gradle
A java/kudu-backup-tools/build.gradle
R java/kudu-backup-tools/src/main/protobuf/backup.proto
R java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/BackupGraph.scala
R java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/BackupIO.scala
R java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup-tools/src/test/resources/log4j.properties
R java/kudu-backup-tools/src/test/scala/org/apache/kudu/backup/TestBackupGraph.scala
M java/kudu-backup/build.gradle
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupUtils.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/settings.gradle
15 files changed, 211 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/13320/2
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 2
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/13320 )
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13320/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13320/1//COMMIT_MSG@12
PS1, Line 12: it’s
its
http://gerrit.cloudera.org:8080/#/c/13320/1/java/gradle/shadow.gradle
File java/gradle/shadow.gradle:
http://gerrit.cloudera.org:8080/#/c/13320/1/java/gradle/shadow.gradle@53
PS1, Line 53: Our shaded library jars always try to pull in the slf4j api from
: // kudu-client
For my own edification, what does this mean? The shaded jars depend on slf4j api, which we don't want to include in the jar but by default will be included, so we are specifically excluding it?
http://gerrit.cloudera.org:8080/#/c/13320/1/java/kudu-backup-tools/build.gradle
File java/kudu-backup-tools/build.gradle:
http://gerrit.cloudera.org:8080/#/c/13320/1/java/kudu-backup-tools/build.gradle@27
PS1, Line 27: dependencies {
As I understand it, this doesn't restrict the versions the tools can work with. E.g., if someone is backing up to HDFS x.y but the tool was built for x.(y + 1), the tool will still work on HDFS x.y as long as we aren't using new features of the x.(y + 1) API. The same thing is true for Spark?
What about Scala and Java? We include Scala of a particular version. Does that limit what Java versions we run on in a way that you think could be limiting, like preventing us from running a tool one or two versions newer against an older Hadoop + Spark cluster?
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 13 May 2019 17:16:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13320 )
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 2
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 14 May 2019 17:20:01 +0000
Gerrit-HasComments: No
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13320 )
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
Patch Set 1:
Should we also include a log4j.properties in main/resources?
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 13 May 2019 18:10:24 +0000
Gerrit-HasComments: No
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/13320 )
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 2
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 14 May 2019 17:12:28 +0000
Gerrit-HasComments: No
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13320 )
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
Patch Set 1:
(4 comments)
Should we also include a log4j.properties in main/resources?
Good question. If we don't, I think the default behavior is to log to stdout.
http://gerrit.cloudera.org:8080/#/c/13320/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13320/1//COMMIT_MSG@12
PS1, Line 12: it’s
> its
Done
http://gerrit.cloudera.org:8080/#/c/13320/1/java/gradle/shadow.gradle
File java/gradle/shadow.gradle:
http://gerrit.cloudera.org:8080/#/c/13320/1/java/gradle/shadow.gradle@53
PS1, Line 53: Our shaded library jars always try to pull in the slf4j api from
: // kudu-client
> For my own edification, what does this mean? The shaded jars depend on slf4
We have two "kinds" of jars in our build. Library jars and tool jars.
A Library jar should shade all non-public API dependencies and leave any dependency that is a part of public API as a dependency. In the case of Slf4j, it is always public API so we don't want to shade it.
However, for a tool jar which is only expected to be used as a standalone application and not a library, we want to included all needed dependencies.
http://gerrit.cloudera.org:8080/#/c/13320/1/java/kudu-backup-tools/build.gradle
File java/kudu-backup-tools/build.gradle:
http://gerrit.cloudera.org:8080/#/c/13320/1/java/kudu-backup-tools/build.gradle@27
PS1, Line 27: dependencies {
> As I understand it, this doesn't restrict the versions the tools can work w
For Hadoop and Spark this is true. It is possible that there is some client/protocol breakage that isn't handled well because we are using a different version. This is the trade off of including the dependencies in the jar as opposed to building the classpath at runtime.
Scala doesn't really matter since this is a tool that wont run with anything else on the classpath. That effectively makes Scala a library.
Java has no extra concerns/considerations than any other jars we produce.
http://gerrit.cloudera.org:8080/#/c/13320/1/java/kudu-backup/build.gradle
File java/kudu-backup/build.gradle:
http://gerrit.cloudera.org:8080/#/c/13320/1/java/kudu-backup/build.gradle@23
PS1, Line 23: compile(project(path: ":kudu-backup-tools")) {
> Don't we need backup-tools to be a leaf dependency if we are shipping it as
There is difference between what we publish and what we use. We always publish the shaded version, but for the sake of code re-use I am making this module depend on the non-shaded version. Gradle provides much more control around this than Maven has.
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 1
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 14 May 2019 12:32:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13320 )
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
Patch Set 1:
> > Should we also include a log4j.properties in main/resources?
>
> Yes please. We currently use the same log4j.properties in each module, and that consistency gives us the right logging for tests.
Sorry, I misunderstood and misread. This patch does include a log4j.properties in src/test/resources; Mike is asking about src/main/resources. I don't have any opinions on that.
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 13 May 2019 18:14:29 +0000
Gerrit-HasComments: No
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13320 )
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
[backup] Factor out kudu-backup-tools module
This patch refactors the kudu-backup module to make
a kudu-backup-tools module. This module will be used
in a future patch to create a more lightweight CLI tool
that shades in all of its dependencies.
While refactoring, I also broke out SessionIO into
BackupIO and BackupUtils.
Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Reviewed-on: http://gerrit.cloudera.org:8080/13320
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley <wd...@gmail.com>
Reviewed-by: Mike Percy <mp...@apache.org>
---
M java/gradle/shadow.gradle
A java/kudu-backup-tools/build.gradle
R java/kudu-backup-tools/src/main/protobuf/backup.proto
R java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/BackupGraph.scala
R java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/BackupIO.scala
R java/kudu-backup-tools/src/main/scala/org/apache/kudu/backup/TableMetadata.scala
A java/kudu-backup-tools/src/test/resources/log4j.properties
R java/kudu-backup-tools/src/test/scala/org/apache/kudu/backup/TestBackupGraph.scala
M java/kudu-backup/build.gradle
A java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupUtils.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/Options.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/settings.gradle
15 files changed, 211 insertions(+), 94 deletions(-)
Approvals:
Kudu Jenkins: Verified
Will Berkeley: Looks good to me, but someone else must approve
Mike Percy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 3
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 (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13320 )
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
Patch Set 1:
(1 comment)
Thanks for creating this module.
http://gerrit.cloudera.org:8080/#/c/13320/1/java/kudu-backup/build.gradle
File java/kudu-backup/build.gradle:
http://gerrit.cloudera.org:8080/#/c/13320/1/java/kudu-backup/build.gradle@23
PS1, Line 23: compile(project(path: ":kudu-backup-tools")) {
Don't we need backup-tools to be a leaf dependency if we are shipping it as a fully shaded fat jar, or that doesn't matter in the Gradle world because we will publish multiple variations of it? (with and without deps)
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 13 May 2019 17:31:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [backup] Factor out kudu-backup-tools module
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13320 )
Change subject: [backup] Factor out kudu-backup-tools module
......................................................................
Patch Set 1:
> Patch Set 1:
>
> Should we also include a log4j.properties in main/resources?
Yes please. We currently use the same log4j.properties in each module, and that consistency gives us the right logging for tests.
--
To view, visit http://gerrit.cloudera.org:8080/13320
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ef2c21fbc31b11b20f0588b6de3cd4998b67443
Gerrit-Change-Number: 13320
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 13 May 2019 18:11:48 +0000
Gerrit-HasComments: No