----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72094/#review219527 -----------------------------------------------------------
Will you be able get this patch committed without waiting for all the dependencies to land? Could you add a bit of your thinking here into the description? I assume it's simply that we don't want callers to be mutating or calling non-const operations on these, but I also wonder if there was something else that triggered this change, so even if the description was just that latter desire, that would be helpful for the reader :) I held off on a ship it because of the addFramework change and some suggestions around that. Let me know your thoughts! src/master/http.cpp Lines 637-653 (original), 637-654 (patched) <https://reviews.apache.org/r/72094/#comment307723> Per below comment, this could just stay almost the same: ``` if (framework->http().isNone()) { ``` ``` if (streamId != framework->http()->streamId.toString()) { ``` src/master/master.hpp Lines 2552-2562 (patched) <https://reviews.apache.org/r/72094/#comment307722> How about: ``` const Option<StreamingHttpConnection<v1::scheduler::Event>& http() const { return http_; } Option<process::UPID> pid() const { return pid_; } ``` isConnectedVia doesn't seem to help over the caller just doing an `f->http == http` (note that option will handle the none case in this equality operator overload): ``` if (framework->isConnectedVia(http)) { // vs if (framework->http == http) { ``` We also won't need many of the other code changes (see my other comments) src/master/master.hpp Lines 2564 (patched) <https://reviews.apache.org/r/72094/#comment307721> Seems a little weird that accessors would return copies, we can avoid it here: ``` const Option<process::UPID>& pid() const { return pid_; } ``` src/master/master.cpp Line 2735 (original), 2735-2736 (patched) <https://reviews.apache.org/r/72094/#comment307724> whoa, what's going on here? seems confusing to me that addFramework now handles the pid disconnection monitoring but not the http disconnection monitoring, see my suggestion below in addFramework src/master/master.cpp Lines 10394-10406 (original), 10395-10399 (patched) <https://reviews.apache.org/r/72094/#comment307725> yikes.. per my suggestion above to expose framework->http, we can keep the disconnection monitoring responsibility within addFramework: ``` if (framework->connected()) { if (framework->pid().isSome()) { link(framework->pid().get()); } else { CHECK_SOME(framework->http()); framework->http()->closed() .onAny(defer(self(), &Self::exited, framework->id(), *framework->http)); } } ``` Alternatively, maybe there's a case to be made to have the disconnection monitoring setup occur outside of addFramework, but that seems like an orthogonal change to this patch. - Benjamin Mahler On Feb. 7, 2020, 5:32 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72094/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2020, 5:32 p.m.) > > > Review request for mesos, Benjamin Mahler and Greg Mann. > > > Bugs: MESOS-10056 > https://issues.apache.org/jira/browse/MESOS-10056 > > > Repository: mesos > > > Description > ------- > > This is a prerequisite to adding one more connection-related state > member in the next patches (ObjectApprovers for framework's principal). > > > Diffs > ----- > > src/master/framework.cpp e69a7c26d15ffffb3d147328032f996962387c96 > src/master/http.cpp eeaac88c893b43170e655f8bff1d57dd0f7bbfb2 > src/master/master.hpp c813e9fc855cfb1701ec32be7f690e06b6eb203f > src/master/master.cpp d41ae724ba12b5ad1c8ae3c1f9b91a05b0e46e7e > src/master/readonly_handler.cpp 40005a2ac2cc9dd6075425c184d220f0a2fbee99 > > > Diff: https://reviews.apache.org/r/72094/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
