Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24374 )
Change subject: KUDU-3781: Add TLS support to MiniHms ...................................................................... Patch Set 2: (9 comments) Thanks for adding this. This looks good to me overall, a few nits and questions. http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/Untitled File src/kudu/hms/Untitled: PS2: What is this? http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.h@50 PS2, Line 50: EnableTls() Even if we don't have a use-case when we re-use the same MiniHms instance for two consecutive runs with and without TLS enabled right now, consider adding a boolean parameter for this method, similar to EnableKuduPlugin() below. http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@268 PS2, Line 268: && env->FileExists(server_key_store) Why is this part necessary? Please add a comment. I'd think that having the success marker/cookie file is enough since it's being created if and only if everything before its creation succeeds. http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@272 PS2, Line 272: for (const auto& path : { ca_key_store, : server_key_store, : ca_cert, : server_csr, : server_cert, : success_marker }) { : if (env->FileExists(path)) { : RETURN_NOT_OK(env->DeleteFile(path)); : } : } Instead, shouldn't this be done on the an automatic/scope clean-up path if anything fails before creating the success cookie file? http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@284 PS2, Line 284: vector<string> args Why to copy the args vector? Could we pass reference to 'args' instead? http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@286 PS2, Line 286: RETURN_NOT_OK(Subprocess::Call(args, "", nullptr, nullptr)); : return Status::OK(); nit: could shorten this to be return Subprocess::Call(args, "", nullptr, nullptr); http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@303 PS2, Line 303: "-keypass", key_store_password_, Ah, interesting: the keytool on macOS worked without requiring password to protect the key. Does HMS apply the store password (provided with the hive.metastore.keystore.password property) as the key password when loading the server certificate from the generated keystore, and everything works as expected? http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@380 PS2, Line 380: WriteStringToFile(env, "ok\n", success_marker); nit: consider creating an empty file instead: it's a bit misleading to add some content into the cookie file when regardless of its contents the only thing that matters is the presence of the file itself. Also, not adding any content might save a few I/O ops and CPU cycles. NewWritableFile(opts, path, &file) with opts.mode = Env::MUST_CREATE and opt.sync_on_close = true should work fine for that? The MUST_CREATE option is a bonus since it helps to guard against various mishaps and bugs when the file is already present when it shouldn't. http://gerrit.cloudera.org:8080/#/c/24374/2/src/kudu/hms/mini_hms.cc@511 PS2, Line 511: static const string kTlsDisabledConfig = R"( : <property> : <name>hive.metastore.use.SSL</name> : <value>false</value> : </property> I'm curious: is it any better than relying on default setting for the ive.metastore.use.SSL property and not adding anything if SSL isn't enabled? -- To view, visit http://gerrit.cloudera.org:8080/24374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fca647fd900fafc6d737c93a4af4565631839b7 Gerrit-Change-Number: 24374 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Thu, 28 May 2026 18:28:14 +0000 Gerrit-HasComments: Yes
