Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18472 )
Change subject: jwt: determine discovery endpoint from token ...................................................................... Patch Set 14: (5 comments) Almost there! A few question on how the newly introduced test behaves if any of the assertion triggers, and some nits. http://gerrit.cloudera.org:8080/#/c/18472/14/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18472/14/src/kudu/util/jwt-util-test.cc@1073 PS14, Line 1073: TEST(JwtUtilTest, VerifyJWKSDiscoveryEnpointMultipleClients) { I'm curious how this test behaves if injecting an assertion into the 'verify' functor? Is that as expected? http://gerrit.cloudera.org:8080/#/c/18472/14/src/kudu/util/jwt-util-test.cc@1087 PS14, Line 1087: verify As per parallel execution, it make sense to gate all the threads with CountDownLatch to make sure all the threads start executing this 'verify' task at once. For example, you could take a look at https://github.com/apache/kudu/blob/f64ed2aac40515ae46132a9bc3cdf7ad5b3f33de/src/kudu/integration-tests/tablet_copy-itest.cc#L344-L358 http://gerrit.cloudera.org:8080/#/c/18472/14/src/kudu/util/jwt-util-test.cc@1093 PS14, Line 1093: const nit: might be constexr ? http://gerrit.cloudera.org:8080/#/c/18472/14/src/kudu/util/jwt-util-test.cc@1099 PS14, Line 1099: std::thread &th nits: (a) there is 'auto' in C++, staring C++11 (b) per C++ style guide used in the project, the asterisk should stick to the type, not the variable http://gerrit.cloudera.org:8080/#/c/18472/14/src/kudu/util/jwt-util-test.cc@1101 PS14, Line 1101: } Does this thread joining work as expected when an assertion is triggered in the 'verify' function? If not, then maybe a good alternative might be joining threads using SCOPED_CLEANUP, something similar to this: https://github.com/apache/kudu/blob/f64ed2aac40515ae46132a9bc3cdf7ad5b3f33de/src/kudu/consensus/log_cache-test.cc#L393-L398 -- To view, visit http://gerrit.cloudera.org:8080/18472 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I970bcc6d894c0206160196418d549b71c35ac973 Gerrit-Change-Number: 18472 Gerrit-PatchSet: 14 Gerrit-Owner: Andrew Wong <[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: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 20 Jan 2023 22:18:12 +0000 Gerrit-HasComments: Yes
