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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 20 Mar 2020 23:35:38 +0000 Gerrit-HasComments: Yes
