Zoltan Chovan has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/24374 )

Change subject: KUDU-3781: Add TLS support to MiniHms
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/24374/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/24374/1//COMMIT_MSG@7
PS1, Line 7: KUDU-3781: Add TLS support to MiniHms
> nit: maybe include the ticket in the title: KUDU-3781?
good idea!


http://gerrit.cloudera.org:8080/#/c/24374/1/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/24374/1/src/kudu/hms/hms_client-test.cc@117
PS1, Line 117:                    enable_tls ? "true" : "false"));
> If any of the earlier assert macros fail, the function returns immediately
good catch, fixed


http://gerrit.cloudera.org:8080/#/c/24374/1/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:

http://gerrit.cloudera.org:8080/#/c/24374/1/src/kudu/hms/mini_hms.cc@268
PS1, Line 268:   if (env->FileExists(success_marker) && 
env->FileExists(server_key_store)) {
             :     return Status::OK();
             :   }
             :
             :   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));
             :     }
             :   }
> Is this caching ever used? Aren't test files cleaned up after test executio
iirc if dist-test retries a test, then it's the same env, so the testfiles are 
there, but maybe I'm mistaken


http://gerrit.cloudera.org:8080/#/c/24374/1/src/kudu/hms/mini_hms.cc@325
PS1, Line 325: genkey
> "This command was named -genkey in earlier releases. The old name is still
Done


http://gerrit.cloudera.org:8080/#/c/24374/1/src/kudu/hms/mini_hms.cc@335
PS1, Line 335: storetype", "
> If i'm not mistaken this should be -storetype. [1] (
Done



--
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 14:51:31 +0000
Gerrit-HasComments: Yes

Reply via email to