----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63732/#review192090 -----------------------------------------------------------
Fix it, then Ship it! src/master/master.cpp Lines 7108-7111 (original), 7111-7117 (patched) <https://reviews.apache.org/r/63732/#comment270107> If agent has resource provider capability, update slave message will be sent to the master from each agent during (re)registration. As a result, the following code will be executed in master for each agent, which is quite expensive. Is there anyway to avoid that massive messages? either from the agent side to not send it if there's no change, or avoid the following code in the master if there is no change. src/master/master.cpp Lines 7108-7111 (original), 7111-7117 (patched) <https://reviews.apache.org/r/63732/#comment270121> I feel the code here will be much more simplified if the master keeps track of Resource Providers for each slave. let's follow up with a patch to clean this up. src/master/master.cpp Lines 7147-7149 (patched) <https://reviews.apache.org/r/63732/#comment270114> Can you just do: ``` resourceProviders[providerId].oldTotal = resources; ``` src/master/master.cpp Lines 7157-7158 (patched) <https://reviews.apache.org/r/63732/#comment270113> Can you just do the following? ``` resourceProviders[providerId].newTotal = resources; ``` src/master/master.cpp Lines 7163 (patched) <https://reviews.apache.org/r/63732/#comment270112> Please fix the style src/master/master.cpp Lines 7174-7175 (patched) <https://reviews.apache.org/r/63732/#comment270111> The indentation here is a bit weird. Either ``` Option<ResourceProviderID> providerId = providerId_.isSome() ? providerId_.get() : Option<ResourceProviderID>::none(); ``` or ``` Option<ResourceProviderID> providerId = providerId_.isSome() ? providerId_.get() : Option<ResourceProviderID>::none(); ``` src/master/master.cpp Lines 7188-7189 (patched) <https://reviews.apache.org/r/63732/#comment270115> Adjust the comments here? src/master/master.cpp Lines 7191 (patched) <https://reviews.apache.org/r/63732/#comment270116> use const ref now? src/master/master.cpp Lines 7217 (patched) <https://reviews.apache.org/r/63732/#comment270117> why use `emplace` above but `insert` here? src/master/master.cpp Lines 7367-7373 (patched) <https://reviews.apache.org/r/63732/#comment270124> It's weird to have the notes here. It `if` condition suggests that this is for operations that master knows but agent does not know. The note is for the case where the master does not know about the operation but the agent knows about the operation. For the latter, we'll just ignore that (and TODO to send ack). Please log a warning there. src/master/master.cpp Lines 7381-7383 (patched) <https://reviews.apache.org/r/63732/#comment270125> See my comments above. It's wierd to have the TODO with the `if` block. Please move this out. src/master/master.cpp Line 7114 (original), 7387-7399 (patched) <https://reviews.apache.org/r/63732/#comment270123> This logic should be in `removeOfferOperation` - Jie Yu On Nov. 28, 2017, 9:32 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63732/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2017, 9:32 p.m.) > > > Review request for mesos, Jie Yu and Jan Schlicht. > > > Bugs: MESOS-8207 > https://issues.apache.org/jira/browse/MESOS-8207 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/master/master.cpp 700e12433b0b66efc3f5dd296711c0f203a13144 > > > Diff: https://reviews.apache.org/r/63732/diff/8/ > > > Testing > ------- > > `make check`, tested as part of https://reviews.apache.org/r/63843/. > > This patch requires `protobuf::isSpeculativeOperation` from > https://reviews.apache.org/r/63751/ which is _not_ part of this review chain > (to avoid multiple dependent RRs). > > > Thanks, > > Benjamin Bannier > >
