Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/22462 )

Change subject: IMPALA-13687: Support shared secret key for cookies
......................................................................


Patch Set 14:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/22462/14/be/src/util/openssl-util.h
File be/src/util/openssl-util.h:

http://gerrit.cloudera.org:8080/#/c/22462/14/be/src/util/openssl-util.h@164
PS14, Line 164:   /// Destructor to shut down the reload thread and signal it 
to stop.
              :   ~AuthenticationHashFromFile() override {
              :     // Signal the reload thread to stop.
              :     if (stop_pipe_write_fd_ >= 0) {
              :       write(stop_pipe_write_fd_, "x", 1); // Write a byte to 
signal shutdown.
              :       close(stop_pipe_write_fd_);
              :     }
              :     if (reload_thread_.joinable()) {
              :       reload_thread_.join();
              :     }
              :   }
Do some of our backend tests exercise this code? I'm wondering if it is worth 
adding test cases in openssl-util-test.


http://gerrit.cloudera.org:8080/#/c/22462/14/be/src/util/openssl-util.cc
File be/src/util/openssl-util.cc:

http://gerrit.cloudera.org:8080/#/c/22462/14/be/src/util/openssl-util.cc@358
PS14, Line 358:         if (Status stat = LoadKey(); !stat.ok()) {
              :           LOG(ERROR) << "Failed to reload authentication key: " 
<< stat;
If something overwrites the key with an invalid file, the current key is kept 
in place and continues to work (but we print to the log). If an impalad starts 
up with a bad key, it fails immediately. That sounds reasonable to me.

Can we add tests cases for those two things? (bad key at startup, overwritten 
with bad key still works)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e2345f771608069407e9dcf7ed4697fc0214e7
Gerrit-Change-Number: 22462
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Jason Fehr <[email protected]>
Gerrit-Reviewer: Joe McDonnell <[email protected]>
Gerrit-Reviewer: Michael Smith <[email protected]>
Gerrit-Reviewer: Pranav Lodha <[email protected]>
Gerrit-Reviewer: Yida Wu <[email protected]>
Gerrit-Comment-Date: Wed, 19 Nov 2025 20:49:18 +0000
Gerrit-HasComments: Yes

Reply via email to