Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15069 )
Change subject: mini-cluster: disallow restarting daemons from other threads ...................................................................... Patch Set 1: (3 comments) > Patch Set 1: Code-Review+2 > > Tempted to say that we should push this restriction down into Subprocess > (https://stackoverflow.com/questions/53601200/calling-fork-on-a-multithreaded-process), > but we're pretty careful with our use of async-signal-safe functions between > fork() and exec(), so maybe it's an unnecessary restriction. Yeah, the main crux of it is make sure the thread that is creating the Subprocess is actually the thread we want. Guaranteeing that from Subprocess itself would be great, though doing it from the maintainers of the Subprocess is probably good enough. http://gerrit.cloudera.org:8080/#/c/15069/1/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/15069/1/src/kudu/mini-cluster/external_mini_cluster.h@617 PS1, Line 617: std::thread::id parent_tid_; > nit: add const? Done http://gerrit.cloudera.org:8080/#/c/15069/1/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/15069/1/src/kudu/mini-cluster/external_mini_cluster.cc@945 PS1, Line 945: parent_tid_ = std::this_thread::get_id(); > nit: move to the initializer list? Done http://gerrit.cloudera.org:8080/#/c/15069/1/src/kudu/mini-cluster/external_mini_cluster.cc@975 PS1, Line 975: << "Process being started from thread " << this_tid << " which is different" : << " from the instantiating thread " << parent_tid_; > nit: probably, Substitute() would provide better readability? Yeah, though thread::id only implements operator<<. Leaving it as is unless you feel strongly about it. -- To view, visit http://gerrit.cloudera.org:8080/15069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I184a01be3e1ac7f60a8b3aedab176dc9138033e0 Gerrit-Change-Number: 15069 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 18 Jan 2020 07:19:15 +0000 Gerrit-HasComments: Yes
