Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18472 )
Change subject: jwt: determine discovery endpoint from token ...................................................................... Patch Set 12: (5 comments) http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc@1086 PS12, Line 1086: string subject; Isn't a race to update the 'subject' from different threads when they apply the 'verify' functor. http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc@1089 PS12, Line 1089: s Does it make sense to add an assertion on the expected result status of the PerAccountKeyBasedJwtVerifier::VerifyToken() call? http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc@1091 PS12, Line 1091: return; > warning: redundant return statement at the end of a function with a void re +1: return isn't needed there http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc@1094 PS12, Line 1094: std::thread first_client(verify); : std::thread second_client(verify); Could these be placed in a container? Also, having more threads than just two (maybe, something about 8?) might have higher chance of exposing races and bugs, if any. http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util.cc@997 PS12, Line 997: std::lock_guard<std::mutex> l(jwt_by_account_id_map_lock_); I'd think moving this to be after initting the newly created helper, to guard only the insertion into the jwt_by_account_id_ dictionary. Why to protect the initting of a new helper instance if every thread creates its own? Once the critical section is shorten, maybe we can start using just a lightweight spinlock instead of a heavy mutex to guard access to the jwt_by_account_id_ dictionary? -- 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: 12 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: Thu, 19 Jan 2023 21:32:57 +0000 Gerrit-HasComments: Yes
