Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22462 )
Change subject: IMPALA-13687: Support shared secret key for cookies ...................................................................... Patch Set 21: (10 comments) http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc@488 PS20, Line 488: // Create a temporary file with a random key. > Done Done http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc@502 PS20, Line 502: vector<uint8_t> buf(buffer_size); > It'll hang. I don't see another good way to handle it. I don't either unfortunately. The test should timeout eventually catching this situation. It should be a very, very rare situation (if it ever happens) anyway. http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util-test.cc@535 PS20, Line 535: while (retries++ < max_retries) { > The first is expecting it not to change, so we need to use a decently long Ack http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util.cc@357 PS20, Line 357: LOG(ERROR) << "select() failed in reload thread: " << strerror(errno); > Possible error cases: Agreed. http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/openssl-util.cc@358 PS20, Line 358: break; > Maybe we should retry for a bit (1 minute?) then make it a fatal error. We should retry (maybe a hidden flag to configure the timeout that defaults to 1 minute). I can see a case where the shared secret file is overwritten with a blank file followed by writing the shared secret resulting in two iterations of this function. If the timeout is exceeded, is there any need to configure if the resulting error is fatal or not? I don't see a reason to make that configurable. Instead, just raise a fatal error. http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/webserver-test.cc File be/src/util/webserver-test.cc: http://gerrit.cloudera.org:8080/#/c/22462/20/be/src/util/webserver-test.cc@a603 PS20, Line 603: > The behavior changed specifically for this in-memory test where we create m Ah, that makes sense. No need to make any more changes. http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java File fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java: http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@1052 PS20, Line 1052: writeCookieSecret(keyFile); > Done Nice! http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java: http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapImpalaShellTest.java@344 PS20, Line 344: // Cookie auth is expected to succeed at least once, possibly many times. > Done Done http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java File fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java: http://gerrit.cloudera.org:8080/#/c/22462/20/fe/src/test/java/org/apache/impala/customcluster/LdapWebserverTest.java@205 PS20, Line 205: writeCookieSecret(keyFile); > Done Done http://gerrit.cloudera.org:8080/#/c/22462/21/fe/src/test/java/org/apache/impala/testutil/TestUtils.java File fe/src/test/java/org/apache/impala/testutil/TestUtils.java: http://gerrit.cloudera.org:8080/#/c/22462/21/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@528 PS21, Line 528: public static <T> T retryUntilSuccess(Callable<T> callable, int maxAttempts, Nice! I like the reusable function. -- 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: 21 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: Thu, 11 Dec 2025 23:14:52 +0000 Gerrit-HasComments: Yes
