Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22056 )
Change subject: [subprocess] KUDU-3624 Fix DoWait thread-safety ...................................................................... Patch Set 3: Code-Review+1 (13 comments) Looks good overall, just a few nits. http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@9 PS2, Line 9: Waitpid nit: waitpid() http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@10 PS2, Line 10: DoWait nit: DoWait() http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@13 PS2, Line 13: KillAndWait nit: KillAndWait() http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@14 PS2, Line 14: DoWait nit: DoWait() http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@15 PS2, Line 15: ExitCheckerThread nit: ExitCheckerThread() http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@16 PS2, Line 16: server.cc:401] nit: remove this part since it doesn't have pairing parenthesis, so it looks pretty strange http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@20 PS2, Line 20: waitpid nit: waitpid() http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@20 PS2, Line 20: evaluates nit: replace with 'calls()' or 'invokes()' http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@22 PS2, Line 22: nit: either remove the extra space or use two spaces after a period elsewhere in the commit message http://gerrit.cloudera.org:8080/#/c/22056/2//COMMIT_MSG@23 PS2, Line 23: parallelly nit: 'concurrently' or 'in parallel' http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess.h File src/kudu/util/subprocess.h: http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess.h@106 PS2, Line 106: Status Wait(int* wait_status = nullptr) WARN_UNUSED_RESULT; Since you have updated the code to allow for multiple threads invoking Wait() and a few other related methods concurrently, could you please add a note about this into the comments? It's worth doing so for corresponding methods, and also add a small blurb in the class-wide comment to mention that this class' interface isn't thread-safe except for a few selected methods. http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess.h@224 PS2, Line 224: cache I guess the 'cache' is backed by atomics, so this isn't quite relevant. Probably, it's better to point that this mutex is used to make sur waitpid() is being called only once? http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: http://gerrit.cloudera.org:8080/#/c/22056/2/src/kudu/util/subprocess.cc@809 PS2, Line 809: lock.lock(); > The problem with extending the protected section is that we need to both su Right -- after looking at this closer, it seems the entities to guard access to waitpid() and updating various state are different semantically, so it would be either necessary to deal with two separate synchronization primitives or use atomics for the state since that might be accessed concurrently. With that, I agree that keeping just one mutex for serializing access to waitpid() and keeping the state in atomics is simpler. -- To view, visit http://gerrit.cloudera.org:8080/22056 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1cb540860b439c26e1c8529123c8b29940d9f84f Gerrit-Change-Number: 22056 Gerrit-PatchSet: 3 Gerrit-Owner: Ádám Bakai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai <[email protected]> Gerrit-Comment-Date: Fri, 22 Nov 2024 02:50:28 +0000 Gerrit-HasComments: Yes
