Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18135 )
Change subject: [asan] improve asan success rate, kerberos context init and destroy ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/security/init.cc File src/kudu/security/init.cc: http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/security/init.cc@171 PS7, Line 171: if (stop_latch_.count() > 0) stop_latch_.CountDown(); You don't need to check for count: it's totally fine and safe to call CountDown() on a latch when it's already 0. http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/security/kinit_context.h File src/kudu/security/kinit_context.h: http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/security/kinit_context.h@45 PS7, Line 45: RenewThrea renewal thread http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/security/kinit_context.h@45 PS7, Line 45: delete nit: delte --> destroying KinitContext might be instantiated on the stack as well, right? http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/security/kinit_context.h@46 PS7, Line 46: Status Kdestroy(); Maybe, make this method private and call it in the destructor? http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/security/kinit_context.h@90 PS7, Line 90: CountDownLatch stop_latch_; : scoped_refptr<Thread> reacquire_thread_; Add comments for this two fields: what are they for, what's the relation between them, etc. http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/server/server_base.cc@873 PS7, Line 873: security::DestroyKerberosForServer() Consider wrapping this call into WARN_NOT_OK(). If logging isn't available at this point already, wrap the call into ignore_result(). http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/18135/7/src/kudu/util/net/socket-test.cc@86 PS7, Line 86: SleepFor(MonoDelta::FromMilliseconds(500)); > Why increase it? +1 Indeed, this change doesn't seem relevant to the essence of this changelist. If it makes some tests more stable, it would make sense to separate this change in its own patch. -- To view, visit http://gerrit.cloudera.org:8080/18135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76a639e35fdf951787f14e0603e73e9e19da6691 Gerrit-Change-Number: 18135 Gerrit-PatchSet: 7 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Sat, 29 Jan 2022 19:30:35 +0000 Gerrit-HasComments: Yes
