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 <[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: Sun, 22 Mar 2020 17:33:27 +0000 Gerrit-HasComments: Yes
