You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Zoltan Chovan (Code Review)" <ge...@cloudera.org> on 2022/06/21 16:14:45 UTC
[kudu-CR] [ranger-kms] part2: mini ranger init
Zoltan Chovan has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18645
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
[ranger-kms] part2: mini_ranger init
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms-test.cc
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
13 files changed, 923 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/1
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 16:
(3 comments)
It seems PS16 somehow overwritten changes made by Zoltan in PS15.
http://gerrit.cloudera.org:8080/#/c/18645/16/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:
http://gerrit.cloudera.org:8080/#/c/18645/16/src/kudu/mini-cluster/external_mini_cluster.h@225
PS16, Line 225: port
nit: part
http://gerrit.cloudera.org:8080/#/c/18645/16/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/18645/16/src/kudu/mini-cluster/external_mini_cluster.cc@20
PS16, Line 20: #include <csignal>
: #include <cstdlib>
:
: #include <algorithm>
: #include <cstdint>
nit: it seems the version in PS15 was more consistent with our current coding style
http://gerrit.cloudera.org:8080/#/c/18645/16/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/18645/16/src/kudu/postgres/mini_postgres.h@72
PS16, Line 72: std::string host() const {
Why to remove the 'const std::string& part'?
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 16
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jul 2022 18:51:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 15:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h@392
PS10, Line 392: postgres::MiniPostgres* postgres() const {
> this class is full of methods with similar "constness" should those be chan
OK, if you prefer this to be uniform in that regard, the const-correctness piece could be fixed in a separate changelist -- totally fine with me.
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h@86
PS10, Line 86: IsRunning
> The goal is to be able to determine if the process was already started up,
Right: my point was that your new method called 'IsRunning()' returns 'true' even after calling 'KillAndWait(SIGTERM)' on the Postgres process. That's why I suggested to rename it into IsStarted().
IsRunning() has semantics of 'the process is running now', and that's not exactly captured by the Subprocess::IsStarted() method.
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h
File src/kudu/ranger-kms/mini_ranger_kms.h:
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@49
PS15, Line 49: std::shared_ptr<postgres::MiniPostgres> mini_pg,
: std::shared_ptr<ranger::MiniRanger> mini_ranger)
nit: misaligned indent
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@51
PS15, Line 51:
nit: this supposed to be 2 + 4 spaces
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@61
PS15, Line 61:
nit: this supposed to be 2 + 4 spaces
Take a look at https://github.com/apache/kudu/blob/8902ca6911b348ec3737da0e5f670726642f9fb0/src/kudu/kserver/kserver.cc#L136-L141 for an example.
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@67
PS15, Line 67:
nit: this supposed to be 2 spaces
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.h@135
PS15, Line 135: RangeKMS
nit: RangerKMS
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc
File src/kudu/ranger-kms/mini_ranger_kms.cc:
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc@148
PS15, Line 148: vector<string> files_to_copy = {
nit: could be 'static const'
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc@149
PS15, Line 149: "dbks-site.xml",
: "install.properties"
: "kms-site.xml"
: "log4j.properties",
: "ranger-kms-policymgr-ssl.xml",
: "ranger-kms-security.xml",
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc@157
PS15, Line 157: auto
nit: could be 'const auto&'
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger-kms/mini_ranger_kms.cc@265
PS15, Line 265: port
Do you want to update the 'port_' member at some point with the value of 'port'?
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger/mini_ranger.h@75
PS15, Line 75: explicit MiniRanger(std::string host, std::shared_ptr<postgres::MiniPostgres> mini_pg) :
: MiniRanger(GetTestDataDirectory(), std::move(host), std::move(mini_pg)) {}
nit: incorrect formatting
For an example of correct one, please check out https://github.com/apache/kudu/blob/8902ca6911b348ec3737da0e5f670726642f9fb0/src/kudu/kserver/kserver.cc#L136-L141
http://gerrit.cloudera.org:8080/#/c/18645/15/src/kudu/ranger/mini_ranger.h@147
PS15, Line 147:
nit: wrong indent
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 15
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jul 2022 15:44:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#9).
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
[ranger-kms] part2: mini_ranger init
* added a new class to represent RangerKMS
* modified mini_pg and mini_ranger so that the mini_pg instance can be
shared between ranger admin and kms
* added mini_ranger_kms to external_mini_cluster
* modified tests where necessary
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M java/kudu-client/src/main/java/org/apache/kudu/client/TimeoutTracker.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMetrics.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
19 files changed, 1,432 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/9
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 16:
> Patch Set 16:
>
> (3 comments)
>
> It seems PS16 somehow overwritten changes made by Zoltan in PS15.
Yea, that's my bad. Working on reverting it and rebasing my change on PS15.
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 16
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jul 2022 18:59:59 +0000
Gerrit-HasComments: No
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 10:
(29 comments)
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h@225
PS10, Line 225: port
part
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h@392
PS10, Line 392: postgres::MiniPostgres* postgres() const {
Here and below: to make this const-correct, either add add 'const' to the return type of remove the 'const' method specifier.
https://isocpp.org/wiki/faq/const-correctness
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.cc@23
PS10, Line 23: #include <algorithm>
: #include <cstdint>
Why is this change?
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h@72
PS10, Line 72: std::string
Could be 'const std::string&' to avoid copying
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h@86
PS10, Line 86: IsRunning
It seems Process::IsStarted() might return 'true' if it's already exited. With that, does it make call this method 'IsStarted' as well or here the point is to check whether the process is currently running?
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h
File src/kudu/ranger-kms/mini_ranger_kms.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@21
PS10, Line 21:
nit: remove the extra line
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@49
PS10, Line 49: explicit
nit: don't need explicit here
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@52
PS10, Line 52:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@62
PS10, Line 62:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@78
PS10, Line 78: GetKeys
nit: could be a constant method?
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@86
PS10, Line 86:
:
nit: too many empty extra lines
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@114
PS10, Line 114:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@128
PS10, Line 128:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@138
PS10, Line 138: const std::string host_;
nit: maybe, add a short doc blurb for 'host_' as well?
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc
File src/kudu/ranger-kms/mini_ranger_kms.cc:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@95
PS10, Line 95:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@104
PS10, Line 104:
nit here and belwo: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@106
PS10, Line 106:
nit for here and below: misaligned indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@137
PS10, Line 137: return Status::OK();
nit: could remove this and just call return at line 132 instead of wrapping WriteStringToFile() into RETURN_NOT_OK
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@154
PS10, Line 154: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "kms-site.xml"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/kms-site.xml"),
: WritableFileOptions()));
:
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "dbks-site.xml"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/dbks-site.xml"),
: WritableFileOptions()));
:
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "log4j.properties"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/log4j.properties"),
: WritableFileOptions()));
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "ranger-kms-policymgr-ssl.xml"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/ranger-kms-policymgr-ssl.xml"),
: WritableFileOptions()));
:
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "ranger-kms-security.xml"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/ranger-kms-security.xml"),
: WritableFileOptions()));
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "ranger-kms-site.xml"),
: JoinPathSegments(web_app_dir,"WEB-INF/classes/conf/ranger-kms-site.xml"),
: WritableFileOptions()));
:
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "install.properties"),
: JoinPathSegments(web_app_dir, "WEB-INF/classes/conf/install.properties"),
: WritableFileOptions()));
Could rewrite this using 'for ()' cycle, defining an array of source file names.
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@201
PS10, Line 201:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@210
PS10, Line 210:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@298
PS10, Line 298:
nit: wrong indent
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@333
PS10, Line 333: curl.FetchURL
What if FetchURL returns non-OK status? Should this be wrapped into RETURN_NOT_OK()?
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h
File src/kudu/ranger-kms/mini_ranger_kms_configs.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@42
PS10, Line 42: const char*
nit: could be 'const char* const'
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@72
PS10, Line 72: kms_home
I'm not sure I see '$4' placeholder in kRangerKMSInstallPropertiesTemplate. Also, it doesn't align with the comment for $4 at kRangerKMSInstallPropertiesTemplate
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@79
PS10, Line 79: const char*
nit for here and below: could be 'constexpr const char* const' ?
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@144
PS10, Line 144: const char *
nit: could be 'constexpr const char* const' ?
Also, per code style guide, should stick the asterisk to the type, not to the variable name.
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger/mini_ranger.cc@21
PS10, Line 21:
nit: remove this extra empty line?
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/util/env_util.cc@21
PS10, Line 21: #include <cerrno>
Why is this change?
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Jul 2022 19:50:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 18: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 18
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jul 2022 21:07:15 +0000
Gerrit-HasComments: No
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#5).
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
[ranger-kms] part2: mini_ranger init
* added a new class to represent RangerKMS
* modified mini_pg and mini_ranger so that the mini_pg instance can be
shared between ranger admin and kms
* added mini_ranger_kms to external_mini_cluster
* modified tests where necessary
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms-test.cc
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
17 files changed, 1,474 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/5
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#10).
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
[ranger-kms] part2: mini_ranger init
* added a new class to represent RangerKMS
* modified mini_pg and mini_ranger so that the mini_pg instance can be
shared between ranger admin and kms
* added mini_ranger_kms to external_mini_cluster
* modified tests where necessary
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
16 files changed, 1,429 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/10
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#6).
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
[ranger-kms] part2: mini_ranger init
* added a new class to represent RangerKMS
* modified mini_pg and mini_ranger so that the mini_pg instance can be
shared between ranger admin and kms
* added mini_ranger_kms to external_mini_cluster
* modified tests where necessary
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms-test.cc
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
17 files changed, 1,527 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/6
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#8).
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
[ranger-kms] part2: mini_ranger init
* added a new class to represent RangerKMS
* modified mini_pg and mini_ranger so that the mini_pg instance can be
shared between ranger admin and kms
* added mini_ranger_kms to external_mini_cluster
* modified tests where necessary
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M java/kudu-client/src/main/java/org/apache/kudu/client/TimeoutTracker.java
M java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessMetrics.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
19 files changed, 1,434 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/8
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has uploaded a new patch set (#17) to the change originally created by Zoltan Chovan. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
[ranger-kms] KUDU-3385 - part2: mini_ranger init
For Kudu to be able to provide data at rest encryption, a separate key
management service can be used for encrypting/decrypting keys.
This patch lays the groundwork for integrating RangerKMS as such a
service, enabling us to create mini test clusters with it, thus making
it possible to test the required interactions properly.
Two existing services mini_ranger and mini_pg were modified, so that the
mini_pg instance could be shared between mini_ranger and mini_ranger_kms
and other future services as well.
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
16 files changed, 1,385 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/17
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 17
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18645/12/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/18645/12/src/kudu/mini-cluster/external_mini_cluster.cc@23
PS12, Line 23: #include <algorithm>
nit: order
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Jul 2022 19:59:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#12).
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
[ranger-kms] KUDU-3385 - part2: mini_ranger init
For Kudu to be able to provide data at rest encryption, a separate key
management service can be used for encrypting/decrypting keys.
This patch lays the groundwork for integrating RangerKMS as such a
service, enabling us to create mini test clusters with it, thus making
it possible to test the required interactions properly.
Two existing services mini_ranger and mini_pg were modified, so that the
mini_pg instance could be shared between mini_ranger and mini_ranger_kms
and other future services as well.
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
16 files changed, 1,429 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/12
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 15: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 15
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jul 2022 14:07:22 +0000
Gerrit-HasComments: No
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
Patch Set 7:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/mini-cluster/external_mini_cluster.h@44
PS7, Line 44: //#include "kudu/ranger-kms/mini_ranger_kms.h"
nit: remove
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/mini-cluster/external_mini_cluster.h@228
PS7, Line 228: // Default: false
nit: add a period at the end
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.h@84
PS7, Line 84: bool firstRun() const { return !Env::Default()->FileExists(pg_root()); }
nit: IsFirstRun()
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.h@86
PS7, Line 86: bool isRunning() const { return process_ && process_->IsStarted(); }
nit: IsRunning()
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.cc@101
PS7, Line 101: Status MiniPostgres::Stop() {
nit: indent
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/CMakeLists.txt
File src/kudu/ranger-kms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/CMakeLists.txt@18
PS7, Line 18: # todo @zchovan maybe we need to add a client for mini kms?
nit: todo(zchovan)
is this still needed?
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.h
File src/kudu/ranger-kms/mini_ranger_kms.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.h@45
PS7, Line 45: namespace ranger_kms {
nit: it should be rangerkms instead
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.h@48
PS7, Line 48: public:
nit: indent should be 1 for public and private, and 2 for the functions
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.h@103
PS7, Line 103: // creates keystores for the KMS
nit: start with a capital letter and add a period at the end (also for the others where it's missing)
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.cc
File src/kudu/ranger-kms/mini_ranger_kms.cc:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.cc@333
PS7, Line 333: // curl.set_auth(CurlAuthType::BASIC, "user1", "t1e2s3t4");
nit: remove
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms_configs.h
File src/kudu/ranger-kms/mini_ranger_kms_configs.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms_configs.h@392
PS7, Line 392: <property>
nit: use spaces instead of tabs (here and below)
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger/mini_ranger.h@122
PS7, Line 122: bool isRunning() const { return process_ && process_->IsStarted(); }
nit: IsRunning()
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger/mini_ranger.h@168
PS7, Line 168: // Directory in which to put all our stuff.
nit: indent
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/util/env_util.h
File src/kudu/util/env_util.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/util/env_util.h@85
PS7, Line 85: Status CopyDirectory(Env* env, const std::string& source_path, const std::string& dest_path,
nit: add doc comment
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 21 Jul 2022 16:39:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#18).
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
[ranger-kms] KUDU-3385 - part2: mini_ranger init
For Kudu to be able to provide data at rest encryption, a separate key
management service can be used for encrypting/decrypting keys.
This patch lays the groundwork for integrating RangerKMS as such a
service, enabling us to create mini test clusters with it, thus making
it possible to test the required interactions properly.
Two existing services mini_ranger and mini_pg were modified, so that the
mini_pg instance could be shared between mini_ranger and mini_ranger_kms
and other future services as well.
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
16 files changed, 1,383 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/18
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 18
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
[ranger-kms] KUDU-3385 - part2: mini_ranger init
For Kudu to be able to provide data at rest encryption, a separate key
management service can be used for encrypting/decrypting keys.
This patch lays the groundwork for integrating RangerKMS as such a
service, enabling us to create mini test clusters with it, thus making
it possible to test the required interactions properly.
Two existing services mini_ranger and mini_pg were modified, so that the
mini_pg instance could be shared between mini_ranger and mini_ranger_kms
and other future services as well.
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Reviewed-on: http://gerrit.cloudera.org:8080/18645
Reviewed-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Attila Bukor <ab...@apache.org>
Tested-by: Kudu Jenkins
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
16 files changed, 1,383 insertions(+), 40 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Attila Bukor: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 19
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has uploaded a new patch set (#16) to the change originally created by Zoltan Chovan. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
[ranger-kms] KUDU-3385 - part2: mini_ranger init
For Kudu to be able to provide data at rest encryption, a separate key
management service can be used for encrypting/decrypting keys.
This patch lays the groundwork for integrating RangerKMS as such a
service, enabling us to create mini test clusters with it, thus making
it possible to test the required interactions properly.
Two existing services mini_ranger and mini_pg were modified, so that the
mini_pg instance could be shared between mini_ranger and mini_ranger_kms
and other future services as well.
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
16 files changed, 1,429 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/16
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 16
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#14).
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
[ranger-kms] KUDU-3385 - part2: mini_ranger init
For Kudu to be able to provide data at rest encryption, a separate key
management service can be used for encrypting/decrypting keys.
This patch lays the groundwork for integrating RangerKMS as such a
service, enabling us to create mini test clusters with it, thus making
it possible to test the required interactions properly.
Two existing services mini_ranger and mini_pg were modified, so that the
mini_pg instance could be shared between mini_ranger and mini_ranger_kms
and other future services as well.
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
16 files changed, 1,385 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/14
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 14
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#7).
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
[ranger-kms] part2: mini_ranger init
* added a new class to represent RangerKMS
* modified mini_pg and mini_ranger so that the mini_pg instance can be
shared between ranger admin and kms
* added mini_ranger_kms to external_mini_cluster
* modified tests where necessary
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms-test.cc
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
17 files changed, 1,525 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/7
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has uploaded a new patch set (#2) to the change originally created by Zoltan Chovan. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
[ranger-kms] part2: mini_ranger init
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms-test.cc
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
13 files changed, 923 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/2
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has uploaded a new patch set (#4) to the change originally created by Zoltan Chovan. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
[ranger-kms] part2: mini_ranger init
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms-test.cc
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
17 files changed, 1,431 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/4
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18645/12/src/kudu/ranger-kms/mini_ranger_kms.cc
File src/kudu/ranger-kms/mini_ranger_kms.cc:
http://gerrit.cloudera.org:8080/#/c/18645/12/src/kudu/ranger-kms/mini_ranger_kms.cc@328
PS12, Line 328: MiniRangerKMS::GetKeys()
Should the response be saved as a result of receiving keys from KMS?
http://gerrit.cloudera.org:8080/#/c/18645/12/src/kudu/ranger-kms/mini_ranger_kms.cc@333
PS12, Line 333: curl.FetchURL
What if FetchURL returns non-OK? Should this be wrapped into RETURN_NOT_OK()?
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Jul 2022 21:59:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18645
to look at the new patch set (#15).
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
[ranger-kms] KUDU-3385 - part2: mini_ranger init
For Kudu to be able to provide data at rest encryption, a separate key
management service can be used for encrypting/decrypting keys.
This patch lays the groundwork for integrating RangerKMS as such a
service, enabling us to create mini test clusters with it, thus making
it possible to test the required interactions properly.
Two existing services mini_ranger and mini_pg were modified, so that the
mini_pg instance could be shared between mini_ranger and mini_ranger_kms
and other future services as well.
Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
---
M CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/postgres/mini_postgres.cc
M src/kudu/postgres/mini_postgres.h
A src/kudu/ranger-kms/CMakeLists.txt
A src/kudu/ranger-kms/mini_ranger_kms.cc
A src/kudu/ranger-kms/mini_ranger_kms.h
A src/kudu/ranger-kms/mini_ranger_kms_configs.h
M src/kudu/ranger/mini_ranger-test.cc
M src/kudu/ranger/mini_ranger.cc
M src/kudu/ranger/mini_ranger.h
M src/kudu/ranger/ranger_client-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
16 files changed, 1,385 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18645/15
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 15
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 18: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 18
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jul 2022 20:45:29 +0000
Gerrit-HasComments: No
[kudu-CR] [ranger-kms] part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] part2: mini_ranger init
......................................................................
Patch Set 7:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/mini-cluster/external_mini_cluster.h@44
PS7, Line 44: //#include "kudu/ranger-kms/mini_ranger_kms.h"
> nit: remove
Ack
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/mini-cluster/external_mini_cluster.h@228
PS7, Line 228: // Default: false
> nit: add a period at the end
Ack
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.h@84
PS7, Line 84: bool firstRun() const { return !Env::Default()->FileExists(pg_root()); }
> nit: IsFirstRun()
Done
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.h@86
PS7, Line 86: bool isRunning() const { return process_ && process_->IsStarted(); }
> nit: IsRunning()
Done
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/postgres/mini_postgres.cc@101
PS7, Line 101: Status MiniPostgres::Stop() {
> nit: indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/CMakeLists.txt
File src/kudu/ranger-kms/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/CMakeLists.txt@18
PS7, Line 18: # todo @zchovan maybe we need to add a client for mini kms?
> nit: todo(zchovan)
nope, thx
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.h
File src/kudu/ranger-kms/mini_ranger_kms.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.h@45
PS7, Line 45: namespace ranger_kms {
> nit: it should be rangerkms instead
Ack
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.h@48
PS7, Line 48: public:
> nit: indent should be 1 for public and private, and 2 for the functions
Done
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.h@103
PS7, Line 103: // creates keystores for the KMS
> nit: start with a capital letter and add a period at the end (also for the
Ack
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.cc
File src/kudu/ranger-kms/mini_ranger_kms.cc:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms.cc@333
PS7, Line 333: // curl.set_auth(CurlAuthType::BASIC, "user1", "t1e2s3t4");
> nit: remove
Done
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms_configs.h
File src/kudu/ranger-kms/mini_ranger_kms_configs.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger-kms/mini_ranger_kms_configs.h@392
PS7, Line 392: <property>
> nit: use spaces instead of tabs (here and below)
Done
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger/mini_ranger.h@122
PS7, Line 122: bool isRunning() const { return process_ && process_->IsStarted(); }
> nit: IsRunning()
Done
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/ranger/mini_ranger.h@168
PS7, Line 168: // Directory in which to put all our stuff.
> nit: indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/util/env_util.h
File src/kudu/util/env_util.h:
http://gerrit.cloudera.org:8080/#/c/18645/7/src/kudu/util/env_util.h@85
PS7, Line 85: Status CopyDirectory(Env* env, const std::string& source_path, const std::string& dest_path,
> nit: add doc comment
Done
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Jul 2022 17:26:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ranger-kms] KUDU-3385 - part2: mini ranger init
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18645 )
Change subject: [ranger-kms] KUDU-3385 - part2: mini_ranger init
......................................................................
Patch Set 13:
(26 comments)
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h@225
PS10, Line 225: port
> part
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.h@392
PS10, Line 392: postgres::MiniPostgres* postgres() const {
> Here and below: to make this const-correct, either add add 'const' to the r
this class is full of methods with similar "constness" should those be changed as well?
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/mini-cluster/external_mini_cluster.cc@23
PS10, Line 23: #include <algorithm>
: #include <cstdint>
> Why is this change?
iwyu on my mac messes these up, restoring it to original shape
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h@72
PS10, Line 72: std::string
> Could be 'const std::string&' to avoid copying
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/postgres/mini_postgres.h@86
PS10, Line 86: IsRunning
> It seems Process::IsStarted() might return 'true' if it's already exited.
The goal is to be able to determine if the process was already started up, so we don't try to start it once more. Postgres fails to start up if it was already running or wasn't shut down properly.
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h
File src/kudu/ranger-kms/mini_ranger_kms.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@21
PS10, Line 21:
> nit: remove the extra line
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@49
PS10, Line 49: explicit
> nit: don't need explicit here
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@52
PS10, Line 52:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@62
PS10, Line 62:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@86
PS10, Line 86:
:
> nit: too many empty extra lines
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@114
PS10, Line 114:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@128
PS10, Line 128:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.h@138
PS10, Line 138: const std::string host_;
> nit: maybe, add a short doc blurb for 'host_' as well?
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc
File src/kudu/ranger-kms/mini_ranger_kms.cc:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@95
PS10, Line 95:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@106
PS10, Line 106:
> nit for here and below: misaligned indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@154
PS10, Line 154: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "kms-site.xml"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/kms-site.xml"),
: WritableFileOptions()));
:
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "dbks-site.xml"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/dbks-site.xml"),
: WritableFileOptions()));
:
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "log4j.properties"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/log4j.properties"),
: WritableFileOptions()));
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "ranger-kms-policymgr-ssl.xml"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/ranger-kms-policymgr-ssl.xml"),
: WritableFileOptions()));
:
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "ranger-kms-security.xml"),
: JoinPathSegments(web_app_dir,
: "WEB-INF/classes/conf/ranger-kms-security.xml"),
: WritableFileOptions()));
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "ranger-kms-site.xml"),
: JoinPathSegments(web_app_dir,"WEB-INF/classes/conf/ranger-kms-site.xml"),
: WritableFileOptions()));
:
: RETURN_NOT_OK(
: env_util::CopyFile(env_,
: JoinPathSegments(kms_home, "install.properties"),
: JoinPathSegments(web_app_dir, "WEB-INF/classes/conf/install.properties"),
: WritableFileOptions()));
> Could rewrite this using 'for ()' cycle, defining an array of source file n
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@201
PS10, Line 201:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@210
PS10, Line 210:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@298
PS10, Line 298:
> nit: wrong indent
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms.cc@333
PS10, Line 333: curl.FetchURL
> What if FetchURL returns non-OK status? Should this be wrapped into RETURN
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h
File src/kudu/ranger-kms/mini_ranger_kms_configs.h:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@42
PS10, Line 42: const char*
> nit: could be 'const char* const'
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@72
PS10, Line 72: kms_home
> I'm not sure I see '$4' placeholder in kRangerKMSInstallPropertiesTemplate.
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@79
PS10, Line 79: const char*
> nit for here and below: could be 'constexpr const char* const' ?
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger-kms/mini_ranger_kms_configs.h@144
PS10, Line 144: const char *
> nit: could be 'constexpr const char* const' ?
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/ranger/mini_ranger.cc@21
PS10, Line 21:
> nit: remove this extra empty line?
Done
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
http://gerrit.cloudera.org:8080/#/c/18645/10/src/kudu/util/env_util.cc@21
PS10, Line 21: #include <cerrno>
> Why is this change?
iwyu, restoring original
--
To view, visit http://gerrit.cloudera.org:8080/18645
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11617468245068dd732fb3f2578bb086b2f6024f
Gerrit-Change-Number: 18645
Gerrit-PatchSet: 13
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Jul 2022 11:42:21 +0000
Gerrit-HasComments: Yes