You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2020/03/05 23:39:49 UTC

[kudu-CR] [WIP] Mini Ranger + Postgres 

Hello Andrew Wong, Anonymous Coward (314), Adar Dembo,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/15374

to review the following change.


Change subject: [WIP] Mini Ranger + Postgres
......................................................................

[WIP] Mini Ranger + Postgres

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
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/ranger/CMakeLists.txt
A src/kudu/ranger/mini_ranger-test.cc
A src/kudu/ranger/mini_ranger.cc
A src/kudu/ranger/mini_ranger.h
M src/kudu/util/subprocess.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
13 files changed, 629 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/1
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (314)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 9: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 17:57:25 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

I also removed the BaseName(argv[0]) call in Subprocess as that doesn't
seem to be the correct behavior according to exec(3) man page as Adar
pointed out.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Reviewed-on: http://gerrit.cloudera.org:8080/15374
Reviewed-by: Hao Hao <ha...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Attila Bukor <ab...@apache.org>
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
19 files changed, 468 insertions(+), 7 deletions(-)

Approvals:
  Hao Hao: Looks good to me, approved
  Adar Dembo: Looks good to me, approved
  Attila Bukor: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 27
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 9: Code-Review+2

Probably, address Tidy's warnings as well?  Maybe Andrew has more feedback.  Anyways, LGTM.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 18:07:46 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#22).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

I also removed the BaseName(argv[0]) call in Subprocess as that doesn't
seem to be the correct behavior according to exec(3) man page as Adar
pointed out.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 457 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/22
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 22
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 26: Verified+1

unrelated flaky test in ASAN


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 26
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Mar 2020 22:01:10 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#17).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 453 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/17
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 17
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#14).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 319 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/14
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 25: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15374/25/src/kudu/postgres/mini_postgres-test.cc
File src/kudu/postgres/mini_postgres-test.cc:

http://gerrit.cloudera.org:8080/#/c/15374/25/src/kudu/postgres/mini_postgres-test.cc@49
PS25, Line 49: restart
Nit: Restart


http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh@1034
PS21, Line 1034:   # We don't need extra features like readline and zlib so so let's just
> fewer system dependencies and presumably faster build, although I didn't be
No, it's fine as-is; just wanted to better understand the effects.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 25
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Mar 2020 20:35:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 26: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 26
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Mar 2020 21:09:36 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 11:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@153
PS9, Line 153: db.Start());
> By default it logs to stderr, but it can be configured to log to file. stde
It would be nice to config to log to the data root dir of minipostgres, similar to other services, as that is where we expect to find the log normally.


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@157
PS9, Line 157: 
             : Status MiniPostgres
> I'm not sure what you mean. Do you think the min_wal_size should be set low
Yeah, mostly about the size of min_wal_size and max_wal_size. Will it ever reach the maximum size? If so, can it be too large  that affects other concurrent running tests? Similarly for min_wal_size, do we need 80MB as a minimal wal size?


http://gerrit.cloudera.org:8080/#/c/15374/11/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/11/src/kudu/ranger/mini_postgres.cc@52
PS11, Line 52:  The kernel won't assign this port until it's in
             : // this state but it can still be used by binding it explicitly.
Not quite following this. 'until it's in this state' refer to which state?  TIME_WAIT?


http://gerrit.cloudera.org:8080/#/c/15374/11/src/kudu/ranger/mini_postgres.cc@160
PS11, Line 160: postgresql.conf
This is the default configuration provided by postgresql? If so, can you add a comment.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 04:44:21 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#24).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

I also removed the BaseName(argv[0]) call in Subprocess as that doesn't
seem to be the correct behavior according to exec(3) man page as Adar
pointed out.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
19 files changed, 461 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/24
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 24
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#8).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 303 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/8
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniRanger pt 1

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed Anonymous Coward (314) from this change.  ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniRanger pt 1
......................................................................


Removed reviewer null.
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#13).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 311 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/13
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#15).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 319 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/15
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102
PS6, Line 102:  so generate a port now.
             :   // TODO(awong): keep trying until we succeed.
> I was thinking about mini clusters that you create manually with "kudu test
It seems this should work.

If not, another (but rather invasive) option might be patching Postgres in thirdparty to set SO_REUSEPORT on its socket and then use MiniCluster::ReserveDaemonSocket() to reserve a socket before starting MiniPostres.  That would be a 100% guarantee to avoid flakiness.  This is how I did it for MiniChronyd.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 17:45:44 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#11).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 309 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/11
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#6).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgrSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1]
https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 297 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/6
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 26:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15374/25/src/kudu/postgres/mini_postgres-test.cc
File src/kudu/postgres/mini_postgres-test.cc:

http://gerrit.cloudera.org:8080/#/c/15374/25/src/kudu/postgres/mini_postgres-test.cc@49
PS25, Line 49: Restart
> Nit: Restart
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh@1034
PS21, Line 1034:   # We don't need extra features like readline and zlib so so let's just
> No, it's fine as-is; just wanted to better understand the effects.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 26
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Mar 2020 20:52:13 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 15:

(20 comments)

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt
File src/kudu/ranger/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt@35
PS15, Line 35: add_library(mini_postgres
I'd actually move mini_postgres into its own module, and add a simple unit test that starts it, does some operation, verifies that it succeeds, and stops it.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h
File src/kudu/ranger/mini_postgres.h:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@32
PS15, Line 32: class MiniPostgres {
Class doc?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@44
PS15, Line 44:   Status AddUser(const std::string& user, bool super);
             :   Status CreateDb(const std::string& db, const std::string& owner);
Should doc some of the finer details of these two (i.e. what effect does 'super' have? Or 'owner'?)


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@51
PS15, Line 51:   std::string pg_root() const {
Nit: add an empty line before.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@52
PS15, Line 52:     DCHECK(!data_root_.empty());
Isn't this guaranteed by the constructor?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@60
PS15, Line 60:  private:
Nit: add an empty line before.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@65
PS15, Line 65:   std::string data_root_;
Could be const?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@69
PS15, Line 69:     Stop();
WARN_NOT_OK.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@79
PS15, Line 79:   string exe;
             :   RETURN_NOT_OK(env->GetExecutablePath(&exe));
             :   bin_dir_ = DirName(exe);
This can be done in the constructor, and then bin_dir_ could be const.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@83
PS15, Line 83:   if (data_root_.empty()) {
             :     data_root_ = GetTestDataDirectory();
             :   }
Can this also be done in the constructor?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@86
PS15, Line 86: pg_root
Nit: maybe call this pgr so you can avoid having to use 'this'?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@97
PS15, Line 97: in this case it doesn't, as we
             :     // BaseName argv[0] in util/Subprocess
I remember we talked about this offline: what did you think about removing that BaseName behavior from the subprocess code?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@101
PS15, Line 101: $$PATH
Won't this be a literal '$PATH' rather than the shell-expanded PATH environment variable? If not, who expands it?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@107
PS15, Line 107:     // Postgres doesn't support binding to 0 so we need to get a random unused
              :     // port and persist that to the config file.
Yeah I'd rather we did this, or we used the UNIQUE_LOOPBACK trick (see net_util.h) to randomly pick a 127.x.y.z loopback address with a statically defined port, thus making collisions extremely unlikely.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@117
PS15, Line 117:   // otherwise it defaults to /usr/lib/postgres
Nit: terminate with a period.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@167
PS15, Line 167: kConfigFile
It's not a constant so it should be named config_file.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@168
PS15, Line 168:   ifstream stream(kConfigFile);
              :   string config((istreambuf_iterator<char>(stream)), istreambuf_iterator<char>());
              :   config.append(Substitute("\nport=$0\n", port_));
Just use ReadFileToString (env.h).


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@174
PS15, Line 174:   RETURN_NOT_OK(file->Flush(WritableFile::FlushMode::FLUSH_SYNC));
Don't need this; we don't care about durability in this context.


http://gerrit.cloudera.org:8080/#/c/15374/15/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/15374/15/thirdparty/build-definitions.sh@1038
PS15, Line 1038:     --without-readline \
               :     --without-zlib
Add a comment rationalizing these?


http://gerrit.cloudera.org:8080/#/c/15374/15/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:

http://gerrit.cloudera.org:8080/#/c/15374/15/thirdparty/build-thirdparty.sh@258
PS15, Line 258:   build_postgres
Just curious, how slow/fast is this build?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 23:35:38 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#10).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 300 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/10
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#23).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

I also removed the BaseName(argv[0]) call in Subprocess as that doesn't
seem to be the correct behavior according to exec(3) man page as Adar
pointed out.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
19 files changed, 458 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/23
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 23
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#20).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 480 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/20
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 20
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniRanger pt 1

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Anonymous Coward (314), 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#3).

Change subject: KUDU-3079 Add MiniRanger pt 1
......................................................................

KUDU-3079 Add MiniRanger pt 1

This patch contains the beginning of the work needed for MiniRanger. It
adds Postgres and Ranger as a dependency. It's able to properly
configure and start Postgres and performs a basic configuration for
Ranger, but it's not yet functioning.

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
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/ranger/CMakeLists.txt
A src/kudu/ranger/mini_ranger-test.cc
A src/kudu/ranger/mini_ranger.cc
A src/kudu/ranger/mini_ranger.h
M src/kudu/util/subprocess.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
13 files changed, 635 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/3
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (314)
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 10:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@45
PS9, Line 45: 
nit: add a comment for the method.


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@105
PS9, Line 105: ess_->SetEnvVars({
nit: can you comment on why this is needed?


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@153
PS9, Line 153: ambuf_iterato
Is there a config for where the log will go?


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@157
PS9, Line 157:   RETURN_NOT_OK(file->Append(config));
             :   RETURN_NOT_OK(fil
Will these config affect the tests environment?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 18:17:35 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 25: Code-Review+2

LGTM, but I'll leave this open in case Adar has more feedback.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 25
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Mar 2020 19:02:09 +0000
Gerrit-HasComments: No

[kudu-CR] [WIP] Mini Ranger + Postgres

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: [WIP] Mini Ranger + Postgres
......................................................................


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:

http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.h@46
PS1, Line 46: int
int16_t ?


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.h@46
PS1, Line 46: ;
const?


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.h@48
PS1, Line 48:   std::unique_ptr<Subprocess> postgres_process_;
> error: no template named 'unique_ptr' in namespace 'std' [clang-diagnostic-
apparently <memory> header is missing


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:

http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc@146
PS1, Line 146: max_connections = 100
Is this the default?  If yes, then why is it necessary to add it in here?


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc@152
PS1, Line 152: lc_messages = 'en_US.UTF-8'                     # locale for system error message
             : lc_monetary = 'en_US.UTF-8'                     # locale for monetary formatting
             : lc_numeric = 'en_US.UTF-8'                      # locale for number formatting
             : lc_time = 'en_US.UTF-8'                         # locale for time formatting
Probably, the defaults are good enough, why to add these?


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc@369
PS1, Line 369: BindAndListen
Maybe, Bind() is enough?


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc@372
PS1, Line 372: listener.Close();
nit: this is not needed since the destructor of the Socket does that



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (314)
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 06 Mar 2020 09:23:13 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 21:

(20 comments)

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres-test.cc
File src/kudu/postgres/mini_postgres-test.cc:

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres-test.cc@28
PS21, Line 28: class PostgresTest : public KuduTest {
> How long does the individual test start/stop sequence take?
It's surprisingly fast, these tests all finish within 1.5 seconds on my Mac.


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres-test.cc@43
PS21, Line 43:   Status createdb = postgres_.CreateDb("testdb", "nonexistentuser");
             :   ASSERT_TRUE(createdb.IsRemoteError());
> Can combine.
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.h@82
PS21, Line 82:     env->GetExecutablePath(&exe);
> nit: CHECK_OK here or log if this fails?
Done


http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:

http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h@53
PS20, Line 53:   // bypasssed[1]
> Nit: terminate with a period.
Done


http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h@55
PS20, Line 55:   // [1] https://www.postgresql.org/docs/12/role-attributes.html
> Nit: this should be 1. <url>
Done


http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h@60
PS20, Line 60:   // owners.[1]
> Nit: move the period to after [1].
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@57
PS21, Line 57: this->
> nit: remove?
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@62
PS21, Line 62: unique_ptr
> nit: this doesn't need to be heap-allocated
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@90
PS21, Line 90: "$0:$$PATH"
> Do we need to evaluate this?
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@117
PS21, Line 117: Substitute("$0", port_)
> nit: could we use IntToString() from gutil/strings/numbers.h instead of Sub
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@133
PS21, Line 133: const string& pg_root
> nit: rather than allowing this to be any path/string, how about taking no a
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt
File src/kudu/ranger/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt@35
PS15, Line 35: ##############################
> I think you should include a full lifecycle test with persistence (i.e. sta
do you think testing persistence with DDL (user creation) is enough or should that do something be something DML?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@97
PS15, Line 97: 
             : 
> From the execvp manpage:
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@107
PS15, Line 107: 
              : 
> By "this" I meant "make postgres support binding to 0".
I'm not sure how we can make it support binding to 0. Seems like an intentional choice, should we try to patch it in thirdparty? I'm not convinced the randomized port approach would cause flakiness as it's effectively the same as binding to 0. We're essentially binding to 0, then release the port and then immediately binding to that port again. The kernel should also guarantee it's not reassigned for 2 minutes unless the defaults were changed on the hosts we're running the tests on.

Anyway, I agree we can tackle this in a follow-up commit if we see it leads to flakiness.


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@556
PS21, Line 556: Status Subprocess::WaitAndCheckExitCode() {
> Should this be reusing GetExitStatus()? If not, why not?
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@558
PS21, Line 558:   Status status = DoWait(&wait_status, BLOCKING);
              :   if (!status.ok()) {
              :     return status;
              :   }
> Could just RETURN_NOT_OK?
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@567
PS21, Line 567: RemoteError
> RuntimeError is more appropriate here.
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@574
PS21, Line 574: 
              :   return Status::IllegalState(Substitute("Stopped with signal $0",
              :                                          WSTOPSIG(wait_status)));
> Should enforce that WIFSTOPPED is true before doing this.
Done


http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh@1034
PS21, Line 1034:   # We don't need extra features like readline and zlib so so let's just
> In what way does this simplify the build? Faster? Fewer system dependencies
fewer system dependencies and presumably faster build, although I didn't benchmark and the build is fast enough. Should I remove and just use the defaults where we don't need to override?


http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh@1035
PS21, Line 1035:   # simplify build
> Nit: terminate with a period.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 21
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 22 Mar 2020 23:39:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#19).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 480 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/19
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 19
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#26).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

I also removed the BaseName(argv[0]) call in Subprocess as that doesn't
seem to be the correct behavior according to exec(3) man page as Adar
pointed out.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
19 files changed, 468 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/26
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 26
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#12).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 308 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/12
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 12
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 25:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15374/24/src/kudu/postgres/mini_postgres-test.cc
File src/kudu/postgres/mini_postgres-test.cc:

http://gerrit.cloudera.org:8080/#/c/15374/24/src/kudu/postgres/mini_postgres-test.cc@48
PS24, Line 48:   ASSERT_OK(postgres_.CreateDb("testdb1", "testuser"));
             :   // restart Postgres to test p
> Wasn't sure why we're restarting here until I read the test name. Maybe add
Done


http://gerrit.cloudera.org:8080/#/c/15374/24/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

http://gerrit.cloudera.org:8080/#/c/15374/24/src/kudu/util/subprocess.cc@563
PS24, Line 563: 1)",
> nit: log the info_str too?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 25
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Mar 2020 11:01:56 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 13:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@153
PS9, Line 153: ();
> Shouldn't logs go be collected by glog? I thought that'd happen automatical
I don't feel strongly about this, but I think it's good to have as much logs as we can in the same file/stderr with the tests instead of having to look at different files.


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@157
PS9, Line 157: Status MiniPostgres::CreateConfigs(const string& pg_root) {
             :   Env* env = Env::D
> Seems like these are the defaults.
Yep, these are all the defaults. I don't want to change the defaults unless we have a reason to. I don't think they would get too large though, Ranger shouldn't store too much info in them for our testing scenarios.


http://gerrit.cloudera.org:8080/#/c/15374/11/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/11/src/kudu/ranger/mini_postgres.cc@52
PS11, Line 52:  be used by binding it explicitly.
             : Status GetRandomPort(uint16_t* port) {
> Not quite following this. 'until it's in this state' refer to which state? 
yep, clarified


http://gerrit.cloudera.org:8080/#/c/15374/11/src/kudu/ranger/mini_postgres.cc@160
PS11, Line 160: 
> This is the default configuration provided by postgresql? If so, can you ad
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 13
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 08:39:36 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 6:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/15374/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15374/6//COMMIT_MSG@17
PS6, Line 17: PostgrSQL
PostgreSQL


http://gerrit.cloudera.org:8080/#/c/15374/6//COMMIT_MSG@28
PS6, Line 28: https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
extra line break?


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@53
PS6, Line 53:   listener.Close();
Is it possible to drop this?  Socket is going out of the scope upon return from the function, so Close() will be called automatically from the destructor.


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@99
PS6, Line 99:         
nit: it seems shift should be 4 spaces in this case?


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102
PS6, Line 102:  so generate a port now.
             :   // TODO(awong): keep trying until we succeed.
It seems this comment is outdated, no?  The port number was generated above at line 94, right?


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@108
PS6, Line 108: SIGQUIT
SIGINT?


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@110
PS6, Line 110: LOG(INFO) << "Postgres bound to " << port_;
Is this still true when !wait.ok() ?


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@115
PS6, Line 115:   RETURN_NOT_OK_PREPEND(process_->KillAndWait(SIGTERM), "failed to stop Postgres");
             :   return Status::OK();
nit: replace with


return process_->KillAndWait(SIGTERM), "failed to stop Postgres");

?


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@146
PS6, Line 146: dynamic_shared_memory_type = posix      # the default is the first option
             : max_wal_size = 1GB
             : min_wal_size = 80MB
             : datestyle = 'iso, mdy'
             : lc_messages = 'en_US.UTF-8'                     # locale for system error message
             : lc_monetary = 'en_US.UTF-8'                     # locale for monetary formatting
             : lc_numeric = 'en_US.UTF-8'                      # locale for number formatting
             : lc_time = 'en_US.UTF-8'                         # locale for time formatting
             : default_text_search_config = 'pg_catalog.english'
If these are non-default settings, could you add a comment w.r.t why the default setting was not good enough?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 04:51:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 14:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102
PS6, Line 102: n_dir_, "postgres")) }
             :     });
> It seems this should work.
That also wouldn't protect against the case where the port wasn't used at the time the config was generated but is used later when we start up Postgres using an existing config file though.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 14
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 22:20:44 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 11:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.h
File src/kudu/ranger/mini_postgres.h:

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.h@35
PS9, Line 35: 
> warning: single-argument constructors must be marked explicit to avoid unin
Done


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102
PS6, Line 102: we need to get a random unused
             :     // port and persist that to the config file
> Different miniclusters should get different directories by virtue of using 
I was thinking about mini clusters that you create manually with "kudu test mini_cluster", I think those will use the same directory. Added a comment to GetRandomPort() about how this works. We have 2 minutes to start up Ranger after running GetRandomPort() which should be more than enough, unless someone lowers tcp_fin_timeout significantly which is discouraged, and even then it's unlikely to cause issues as the kernel would need to exhaust the ephemeral port range first.

 // Gets a random port from the ephemeral range by binding to port 0 and letting
 // the kernel choose an unused one from the ephemeral port range. The socket is
 // then immediately closed and it remains in TIME_WAIT for 2*tcp_fin_timeout (by
 // default 2*60=120 seconds). The kernel won't assign this port until it's in
 // this state but it can still be used by binding it explicitly.


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@146
PS6, Line 146: 
             : Status MiniPostgres::CreateDb(const string& db, const string& owner) {
             :   Subprocess createdb({
             :     JoinPathSegments(bin_dir_, "postgres/createdb"),
             :     "-O", owner, db,
             :     "-p", Substitute("$0", port_),
             :   });
             :   RETURN_NOT_OK(createdb.Start());
             :   return createdb.Wait();
> initdb creates these, added a comment
removed this part


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@45
PS9, Line 45: namespace kudu {
> nit: add a comment for the method.
Done


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@105
PS9, Line 105: TURN_NOT_OK(CreateConfigs(pg_root));
> nit: can you comment on why this is needed?
Done


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@153
PS9, Line 153: db.Start());
> Is there a config for where the log will go?
By default it logs to stderr, but it can be configured to log to file. stderr should be fine for us, what do you think?


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@157
PS9, Line 157: 
             : Status MiniPostgres
> Will these config affect the tests environment?
I'm not sure what you mean. Do you think the min_wal_size should be set lower?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 21:02:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 21:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres-test.cc
File src/kudu/postgres/mini_postgres-test.cc:

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres-test.cc@28
PS21, Line 28: class PostgresTest : public KuduTest {
How long does the individual test start/stop sequence take?


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres-test.cc@43
PS21, Line 43:   Status createdb = postgres_.CreateDb("testdb", "nonexistentuser");
             :   ASSERT_TRUE(createdb.IsRemoteError());
Can combine.


http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:

http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h@53
PS20, Line 53:   // bypasssed[1]
Nit: terminate with a period.


http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h@55
PS20, Line 55:   // [1] https://www.postgresql.org/docs/12/role-attributes.html
Nit: this should be 1. <url>

(obviously not a big deal, but that's the style I think you're aiming for)


http://gerrit.cloudera.org:8080/#/c/15374/20/src/kudu/postgres/mini_postgres.h@60
PS20, Line 60:   // owners.[1]
Nit: move the period to after [1].


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt
File src/kudu/ranger/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt@35
PS15, Line 35: ##############################
> did you also mean moving it to src/postgres? added tests that add users and
I think you should include a full lifecycle test with persistence (i.e. start, do something, restart, verify that something was still done).


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@97
PS15, Line 97: 
             : 
> Yeah at the time I thought this was an issue with everything, but as it tur
From the execvp manpage:

       The const char *arg and subsequent ellipses in the execl(), execlp(), and execle() functions
       can be thought of as arg0, arg1, ..., argn.  Together they describe a list of  one  or  more
       pointers  to  null-terminated strings that represent the argument list available to the exe?
       cuted program.  The first argument, by convention, should point to the  filename  associated
       with  the  file being executed.  The list of arguments must be terminated by a null pointer,
       and, since these are variadic functions, this pointer must be cast (char *) NULL.

       The execv(), execvp(), and execvpe() functions provide an array of pointers  to  null-termi?
       nated  strings  that  represent  the  argument list available to the new program.  The first
       argument, by convention, should point to the filename associated with the  file  being  exe?
       cuted.  The array of pointers must be terminated by a null pointer.

Seems like our behavior is wrong and we should ensure the first argument really is the full path to the file being executed.

I'm fine with you tackling this in a follow-up though.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@101
PS15, Line 101: 
> hm good point, I don't think we need the full $PATH here though, so it shou
Bear in mind that if PATH is constrained to just <bin_dir>/postgres, if Postgres does any PATH-based execution of its own (i.e. another subprocess), it probably won't find what it's looking for. Do you expect that to happen?

You could evaluate the current process' $PATH, add <bin_dir>/postgres to it, and use the result here.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@107
PS15, Line 107: 
              : 
> seems UNIQUE_LOOPBACK is not supported on MacOS, at least according to the 
By "this" I meant "make postgres support binding to 0".

Anyway, just because macOS doesn't support something that Linux doesn't mean we shouldn't do it. If you want to tackle this post-merge that's fine, but I'll be surprised if the randomized port approach doesn't cause at least some test flakiness when considering how many tests we run.


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@558
PS21, Line 558:   Status status = DoWait(&wait_status, BLOCKING);
              :   if (!status.ok()) {
              :     return status;
              :   }
Could just RETURN_NOT_OK?


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@567
PS21, Line 567: RemoteError
RuntimeError is more appropriate here.


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@574
PS21, Line 574: 
              :   return Status::IllegalState(Substitute("Stopped with signal $0",
              :                                          WSTOPSIG(wait_status)));
Should enforce that WIFSTOPPED is true before doing this.


http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh@1034
PS21, Line 1034:   # We don't need extra features like readline and zlib so so let's just
In what way does this simplify the build? Faster? Fewer system dependencies? Just curious.


http://gerrit.cloudera.org:8080/#/c/15374/21/thirdparty/build-definitions.sh@1035
PS21, Line 1035:   # simplify build
Nit: terminate with a period.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 21
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 22 Mar 2020 22:04:58 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 24:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15374/24/src/kudu/postgres/mini_postgres-test.cc
File src/kudu/postgres/mini_postgres-test.cc:

http://gerrit.cloudera.org:8080/#/c/15374/24/src/kudu/postgres/mini_postgres-test.cc@48
PS24, Line 48:   ASSERT_OK(postgres_.Stop());
             :   ASSERT_OK(postgres_.Start());
Wasn't sure why we're restarting here until I read the test name. Maybe add some comments?

Also maybe CreateDb() before restarting, and then restart, and then CreateDb() a different database? Then we get coverage both with and without persistence.


http://gerrit.cloudera.org:8080/#/c/15374/24/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

http://gerrit.cloudera.org:8080/#/c/15374/24/src/kudu/util/subprocess.cc@563
PS24, Line 563: exit_status
nit: log the info_str too?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 24
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Mar 2020 07:55:00 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 6:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/15374/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/15374/6//COMMIT_MSG@17
PS6, Line 17: PostgrSQL
> PostgreSQL
Done


http://gerrit.cloudera.org:8080/#/c/15374/6//COMMIT_MSG@28
PS6, Line 28: https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
> extra line break?
Done


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@53
PS6, Line 53:   listener.Close();
> Is it possible to drop this?  Socket is going out of the scope upon return 
Done


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@99
PS6, Line 99:         
> nit: it seems shift should be 4 spaces in this case?
Done


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102
PS6, Line 102:  so generate a port now.
             :   // TODO(awong): keep trying until we succeed.
> It seems this comment is outdated, no?  The port number was generated above
Done


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@108
PS6, Line 108: SIGQUIT
> SIGINT?
Done


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@110
PS6, Line 110: LOG(INFO) << "Postgres bound to " << port_;
> Is this still true when !wait.ok() ?
Done


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@115
PS6, Line 115:   RETURN_NOT_OK_PREPEND(process_->KillAndWait(SIGTERM), "failed to stop Postgres");
             :   return Status::OK();
> nit: replace with
Done


http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@146
PS6, Line 146: dynamic_shared_memory_type = posix      # the default is the first option
             : max_wal_size = 1GB
             : min_wal_size = 80MB
             : datestyle = 'iso, mdy'
             : lc_messages = 'en_US.UTF-8'                     # locale for system error message
             : lc_monetary = 'en_US.UTF-8'                     # locale for monetary formatting
             : lc_numeric = 'en_US.UTF-8'                      # locale for number formatting
             : lc_time = 'en_US.UTF-8'                         # locale for time formatting
             : default_text_search_config = 'pg_catalog.english'
> If these are non-default settings, could you add a comment w.r.t why the de
initdb creates these, added a comment



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 08:00:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#21).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 478 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/21
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 21
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#4).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgrSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1]
https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 292 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/4
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] [WIP] Mini Ranger + Postgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: [WIP] Mini Ranger + Postgres
......................................................................


Patch Set 1:

> Uploaded patch set 1.

I'm getting close, but Ranger still fails to start. I'll investigate it further and fix it, but uploaded it in case anyone feels like playing around with it.

Sorry for this unreadable mess, this is the result of days of trial & error, I'll clean it up once it works.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (314)
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 05 Mar 2020 23:41:25 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102
PS6, Line 102: 
             :       JoinPathSegments(bin_dir_, "postgres/post
> They should each get a new random port as the kernel won't assign the same 
Different miniclusters should get different directories by virtue of using GetTestDataDirectory() (assuming only one is started per test case).

My concern was mainly around what happens if we bind to port 1234 in TestFoo's call to GetRandomPort(), we then write that to disk, and while we're writing, TestFoo is no longer bound to the port and so TestBar's call to GetRandomPort() binds to port 1234 too. Or do we have a guarantee from the kernel that in between GetRandomPort() and starting up postgres that other process won't bind to the same port?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 19:53:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#18).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 472 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/18
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 18
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 8: Code-Review+2

Thank you for putting this together!

LGTM.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 16:27:05 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#5).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgrSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1]
https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 293 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/5
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#7).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 303 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/7
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 7
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 15:

(20 comments)

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt
File src/kudu/ranger/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/CMakeLists.txt@35
PS15, Line 35: add_library(mini_postgres
> I'd actually move mini_postgres into its own module, and add a simple unit 
did you also mean moving it to src/postgres? added tests that add users and databases, do you think that's enough or should I also connect to postgres directly using pg libs?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h
File src/kudu/ranger/mini_postgres.h:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@32
PS15, Line 32: class MiniPostgres {
> Class doc?
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@44
PS15, Line 44:   Status AddUser(const std::string& user, bool super);
             :   Status CreateDb(const std::string& db, const std::string& owner);
> Should doc some of the finer details of these two (i.e. what effect does 's
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@51
PS15, Line 51:   std::string pg_root() const {
> Nit: add an empty line before.
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@52
PS15, Line 52:     DCHECK(!data_root_.empty());
> Isn't this guaranteed by the constructor?
it is now


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@60
PS15, Line 60:  private:
> Nit: add an empty line before.
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.h@65
PS15, Line 65:   std::string data_root_;
> Could be const?
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@69
PS15, Line 69:     Stop();
> WARN_NOT_OK.
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@79
PS15, Line 79:   string exe;
             :   RETURN_NOT_OK(env->GetExecutablePath(&exe));
             :   bin_dir_ = DirName(exe);
> This can be done in the constructor, and then bin_dir_ could be const.
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@83
PS15, Line 83:   if (data_root_.empty()) {
             :     data_root_ = GetTestDataDirectory();
             :   }
> Can this also be done in the constructor?
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@86
PS15, Line 86: pg_root
> Nit: maybe call this pgr so you can avoid having to use 'this'?
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@97
PS15, Line 97: in this case it doesn't, as we
             :     // BaseName argv[0] in util/Subprocess
> I remember we talked about this offline: what did you think about removing 
Yeah at the time I thought this was an issue with everything, but as it turns out this is postgres-specific, so I reverted the removal of BaseName() and added this workaround instead. Do you think I should remove the BaseName() call instead?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@101
PS15, Line 101: $$PATH
> Won't this be a literal '$PATH' rather than the shell-expanded PATH environ
hm good point, I don't think we need the full $PATH here though, so it should be fine to just overwrite it.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@107
PS15, Line 107:     // Postgres doesn't support binding to 0 so we need to get a random unused
              :     // port and persist that to the config file.
seems UNIQUE_LOOPBACK is not supported on MacOS, at least according to the comment in net_utils.h. While the space is much less with the ephemeral ports I think it's still very unlikely.

> Yeah I'd rather we did this

I'm not sure what you mean by "this", did you mean to reference Alexey's comment?


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@117
PS15, Line 117:   // otherwise it defaults to /usr/lib/postgres
> Nit: terminate with a period.
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@167
PS15, Line 167: kConfigFile
> It's not a constant so it should be named config_file.
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@168
PS15, Line 168:   ifstream stream(kConfigFile);
              :   string config((istreambuf_iterator<char>(stream)), istreambuf_iterator<char>());
              :   config.append(Substitute("\nport=$0\n", port_));
> Just use ReadFileToString (env.h).
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@174
PS15, Line 174:   RETURN_NOT_OK(file->Flush(WritableFile::FlushMode::FLUSH_SYNC));
> Don't need this; we don't care about durability in this context.
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/15374/15/thirdparty/build-definitions.sh@1038
PS15, Line 1038:     --without-readline \
               :     --without-zlib
> Add a comment rationalizing these?
Done


http://gerrit.cloudera.org:8080/#/c/15374/15/thirdparty/build-thirdparty.sh
File thirdparty/build-thirdparty.sh:

http://gerrit.cloudera.org:8080/#/c/15374/15/thirdparty/build-thirdparty.sh@258
PS15, Line 258:   build_postgres
> Just curious, how slow/fast is this build?
This is on my Mac after running ccache -C

 ./configure 
 real    0m57.082s
 user    0m15.813s
 sys     0m19.612s

 make
 real    0m7.340s
 user    0m4.891s
 sys     0m5.555s



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 15
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 22 Mar 2020 17:33:27 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 26
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102
PS6, Line 102: 
             : 
> Done
I was and am worried that this would be flaky. If we run this from multiple tests concurrently, could multiple processes attempt to bond to the same port?


http://gerrit.cloudera.org:8080/#/c/15374/8/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/8/src/kudu/ranger/mini_postgres.cc@81
PS8, Line 81:     // need to prepend $PATH with bin_dir_ as initdb uses argv[0] to determine
Nit: capitalize and make a complete sentence?


http://gerrit.cloudera.org:8080/#/c/15374/8/src/kudu/ranger/mini_postgres.cc@143
PS8, Line 143:   // The file template is copied from postgresql.conf generated by initdb. We
Nit: would be nice to read this file and just add a new line for the port. Is that possible?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 8
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 16:41:51 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 21:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.h
File src/kudu/postgres/mini_postgres.h:

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.h@82
PS21, Line 82:     env->GetExecutablePath(&exe);
nit: CHECK_OK here or log if this fails?


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc
File src/kudu/postgres/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@57
PS21, Line 57: this->
nit: remove?


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@62
PS21, Line 62: unique_ptr
nit: this doesn't need to be heap-allocated


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@90
PS21, Line 90: "$0:$$PATH"
Do we need to evaluate this?


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@117
PS21, Line 117: Substitute("$0", port_)
nit: could we use IntToString() from gutil/strings/numbers.h instead of Substitute? Same elsewhere.


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/postgres/mini_postgres.cc@133
PS21, Line 133: const string& pg_root
nit: rather than allowing this to be any path/string, how about taking no args and calling pg_root() in this method? That way it's harder to misuse accidentally.


http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/15/src/kudu/ranger/mini_postgres.cc@97
PS15, Line 97: 
             : 
> From the execvp manpage:
Ah, when I read through that page I assumed it was referring to the first argument of execvp ('path'), rather than the first element of 'arg', but it's pretty clearly talking about *arg here.


http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc
File src/kudu/util/subprocess.cc:

http://gerrit.cloudera.org:8080/#/c/15374/21/src/kudu/util/subprocess.cc@556
PS21, Line 556: Status Subprocess::WaitAndCheckExitCode() {
Should this be reusing GetExitStatus()? If not, why not?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 21
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 22 Mar 2020 22:52:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 26: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 26
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 23 Mar 2020 21:26:24 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@153
PS9, Line 153: db.Start());
> It would be nice to config to log to the data root dir of minipostgres, sim
Shouldn't logs go be collected by glog? I thought that'd happen automatically by virtue of running as a Subprocess (assuming we've set stdout and stderr on the Subprocess).


http://gerrit.cloudera.org:8080/#/c/15374/9/src/kudu/ranger/mini_postgres.cc@157
PS9, Line 157: 
             : Status MiniPostgres
> Yeah, mostly about the size of min_wal_size and max_wal_size. Will it ever 
Seems like these are the defaults.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 11
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 04:48:34 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#9).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M src/kudu/ranger/CMakeLists.txt
A src/kudu/ranger/mini_postgres.cc
A src/kudu/ranger/mini_postgres.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
8 files changed, 313 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/9
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 9
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniRanger pt 1

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniRanger pt 1
......................................................................


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.h
File src/kudu/ranger/mini_ranger.h:

http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.h@46
PS1, Line 46: C
> const?
made it static


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.h@46
PS1, Line 46: Sta
> int16_t ?
Done


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.h@48
PS1, Line 48:   static int16_t GetRandomPort();
> apparently <memory> header is missing
Done


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc
File src/kudu/ranger/mini_ranger.cc:

http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc@146
PS1, Line 146: max_connections = 100
> Is this the default?  If yes, then why is it necessary to add it in here?
initdb creates this config file by default, I pasted it to the template as we need to rewrite the port manually.


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc@152
PS1, Line 152: lc_messages = 'en_US.UTF-8'                     # locale for system error message
             : lc_monetary = 'en_US.UTF-8'                     # locale for monetary formatting
             : lc_numeric = 'en_US.UTF-8'                      # locale for number formatting
             : lc_time = 'en_US.UTF-8'                         # locale for time formatting
> Probably, the defaults are good enough, why to add these?
same as above


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc@369
PS1, Line 369: Bind(address)
> Maybe, Bind() is enough?
Done


http://gerrit.cloudera.org:8080/#/c/15374/1/src/kudu/ranger/mini_ranger.cc@372
PS1, Line 372: 
> nit: this is not needed since the destructor of the Socket does that
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (314)
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 18 Mar 2020 21:36:42 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#25).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

I also removed the BaseName(argv[0]) call in Subprocess as that doesn't
seem to be the correct behavior according to exec(3) man page as Adar
pointed out.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/run_dist_test.py
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
19 files changed, 468 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/25
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 25
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15374

to look at the new patch set (#16).

Change subject: KUDU-3079 Add MiniPostgres
......................................................................

KUDU-3079 Add MiniPostgres

Apache Ranger needs a database to store privileges and supported
databases currently are MySQL, PostgreSQL, Oracle and Amazon RDS[1].

I ruled out Amazon RDS on the basis that it's cloud-based so we couldn't
depend on it in integration tests in a self-contained way.

This is only a build-time dependency (test-time really) and only for an
optional feature, so licensing shouldn't be too much of a concern for
any of them, but PostgreSQL License can even be included in ASF
software[2]. It's also easy to build and configure and it doesn't
otherwise matter which database is used under Ranger, so I went with
Postgres.

This patch adds the latest (at the time of the start of this work)
PostgreSQL and PostgreSQL JDBC driver JARs to thirdparty and adds a
MiniPostgres interface that tests can use to start Postgres, add users
and create databases.

The patch also adds two utility methods that MiniPostgres uses:

- Subprocess::WaitAndCheckExitCode() is the same as Wait(), but it only
  returns OK if the exit code is 0. If the subprocess was killed, stopped
  or exited with a non-zero exit code, it fails.
- GetRandomPort() in net_util.h that returns a random unused port.

[1] https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.5/installing-ranger/content/configuring_a_database_instance_for_ranger.html
(couldn't find a list of supported databases on ranger.apache.org or on
cwiki.apache.org)
[2] https://www.apache.org/legal/resolved.html#category-a

Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
---
M CMakeLists.txt
A src/kudu/postgres/CMakeLists.txt
A src/kudu/postgres/mini_postgres-test.cc
A src/kudu/postgres/mini_postgres.cc
A src/kudu/postgres/mini_postgres.h
M src/kudu/ranger/CMakeLists.txt
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/net/net_util.h
M src/kudu/util/subprocess.cc
M src/kudu/util/subprocess.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 455 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/15374/16
-- 
To view, visit http://gerrit.cloudera.org:8080/15374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-3079 Add MiniPostgres

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15374 )

Change subject: KUDU-3079 Add MiniPostgres
......................................................................


Patch Set 10:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/6/src/kudu/ranger/mini_postgres.cc@102
PS6, Line 102: 
             :       JoinPathSegments(bin_dir_, "postgres/post
> I was and am worried that this would be flaky. If we run this from multiple
They should each get a new random port as the kernel won't assign the same ephemeral port until it's in TIME_WAIT (IIRC 120s by default). Only issue that I can see with this is that it's persisted to the config directory, so if we reuse the same conf dir (e.g. in a MiniCluster) it might be occupied. I added a todo to fix this.


http://gerrit.cloudera.org:8080/#/c/15374/8/src/kudu/ranger/mini_postgres.cc
File src/kudu/ranger/mini_postgres.cc:

http://gerrit.cloudera.org:8080/#/c/15374/8/src/kudu/ranger/mini_postgres.cc@81
PS8, Line 81:       JoinPathSegments(bin_dir_, "postgres/initdb"),
> Nit: capitalize and make a complete sentence?
Done


http://gerrit.cloudera.org:8080/#/c/15374/8/src/kudu/ranger/mini_postgres.cc@143
PS8, Line 143:     "-p", Substitute("$0", port_),
> Nit: would be nice to read this file and just add a new line for the port. 
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba40126aa60e0ecbc5ae10cc1328493194c345bc
Gerrit-Change-Number: 15374
Gerrit-PatchSet: 10
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 19 Mar 2020 18:13:40 +0000
Gerrit-HasComments: Yes