Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20708 )
Change subject: [messenger] Initialize SASL proto name to principal ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/20708/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20708/1//COMMIT_MSG@16 PS1, Line 16: However this change is not enough because ServerBase::ShutdownImpl called : DestroyKerberosForServer which destroyed the global state which : MessengerBuilder uses. And this affects only mini-cluster tests, so for stand-alone processes that would be enough, correct? http://gerrit.cloudera.org:8080/#/c/20708/1//COMMIT_MSG@18 PS1, Line 18: The function call was removed, and since the : function is not used anywhere else, the function was removed as well. Looking at changelist f4fa4d22f (the patch was about getting rid of ASAN warnings), it seems the call to DestroyKerberosForServer() was introduced into ServerBase::ShutdownImpl() to address ASAN warnings that were popping up intermittently. Have you verified that ASAN isn't reporting any warnings when running various tests with this patch? Usually, it requires many runs to see the warning, and adding --stress_cpu_threads into the mix might help to see the warning more often. http://gerrit.cloudera.org:8080/#/c/20708/1/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/20708/1/src/kudu/integration-tests/security-itest.cc@878 PS1, Line 878: const string kPrincipal = "oryx"; nit: this is used only once, so why to introduce a constant then? Even if introducing a constant here, consider using 'const char* const' and add the 'constexpr' specifier to make it a compile-time constant http://gerrit.cloudera.org:8080/#/c/20708/1/src/kudu/integration-tests/security-itest.cc@888 PS1, Line 888: cluster_->tablet_server(0)->Shutdown(); : cluster_->tablet_server(1)->Shutdown(); : cluster_->tablet_server(2)->Shutdown(); Consider converting this into a 'for()' cycle, using ExternalMiniCluster::num_tablet_servers() to deduce the number of loops. http://gerrit.cloudera.org:8080/#/c/20708/1/src/kudu/integration-tests/security-itest.cc@892 PS1, Line 892: ASSERT_OK(cluster_->tablet_server(0)->Restart()); : ASSERT_OK(cluster_->tablet_server(1)->Restart()); : ASSERT_OK(cluster_->tablet_server(2)->Restart()); ditto http://gerrit.cloudera.org:8080/#/c/20708/1/src/kudu/integration-tests/security-itest.cc@898 PS1, Line 898: cluster_->master(0)->Shutdown(); : ASSERT_OK(cluster_->CreateClient(nullptr, &client)); : SmokeTestCluster(client); It would be great to add a comment what's the reason of doing this. -- To view, visit http://gerrit.cloudera.org:8080/20708 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie8decbd0b3e54df42bb0b9b14fc5ec291cd70b8b Gerrit-Change-Number: 20708 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 15 Nov 2023 15:37:33 +0000 Gerrit-HasComments: Yes
