----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65591/#review198741 -----------------------------------------------------------
Thanks Benjamin - this is a nice improvement! src/master/master.cpp Lines 7324 (patched) <https://reviews.apache.org/r/65591/#comment278967> Is there any case where `message.has_resource_version_uuid() && slave->resourceVersion.isNone()` will be true? I'm wondering if we should do something like: ``` if (!updated && message.has_resource_version_uuid()) { CHECK_SOME(slave->resourceVersion); if (message.resource_version_uuid() != slave->resourceVersion.get()) { updated = true; } } ``` src/master/master.cpp Lines 7324-7325 (original), 7361-7362 (patched) <https://reviews.apache.org/r/65591/#comment278968> Suggestion: perhaps using names like `receivedProvider` and `storedProvider` (or `oldProvider`/`newProvider`?) might improve readability. So we would have checks like ``` if (receivedProvider.info != storedProvider.info() ... ``` src/master/master.cpp Line 7492 (original), 7433 (patched) <https://reviews.apache.org/r/65591/#comment278989> Nit: newline after this comment, since it applies to multiple blocks of code below. src/master/master.cpp Lines 7525 (patched) <https://reviews.apache.org/r/65591/#comment278990> s/ways/ways of/ src/master/master.cpp Lines 7530-7538 (patched) <https://reviews.apache.org/r/65591/#comment278991> `framework == nullptr` can represent one of two cases at this point: 1) The operation did not have a framework ID set 2) The framework specified by the operation does not have its info stored in the master In both cases, I believe we want to continue. In only the second case do we want to log the warning. src/master/master.cpp Lines 7635-7640 (original), 7596-7599 (patched) <https://reviews.apache.org/r/65591/#comment278992> Nit: I think you could remove the newline here. src/master/master.cpp Line 7722 (original), 7680-7681 (patched) <https://reviews.apache.org/r/65591/#comment279001> Perhaps we also want to rescind offers when `message.has_resource_providers()` is false? In that case, the agent has told us that it has no registered RPs, so offers with RP resources would not be valid? src/master/master.cpp Lines 10572-10574 (original), 10534-10536 (patched) <https://reviews.apache.org/r/65591/#comment279003> Are you sure this will always be true? Could a framework send us a well-formed, but unknown RP ID in a message and make this false? src/master/master.cpp Lines 11515 (patched) <https://reviews.apache.org/r/65591/#comment279004> Ditto with this CHECK - are we sure that a well-formed but unknown RP ID wouldn't hit this? - Greg Mann On March 5, 2018, 10:21 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65591/ > ----------------------------------------------------------- > > (Updated March 5, 2018, 10:21 p.m.) > > > Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht. > > > Bugs: MESOS-8382 > https://issues.apache.org/jira/browse/MESOS-8382 > > > Repository: mesos > > > Description > ------- > > This patch adds explicit tracking of resource providers to the master > process. While we already had explicitly send resource provider > information in e.g., `UpdateSlaveMessage`, we only stored that > information aggregated over the full agent in the master up to now. > > > Diffs > ----- > > src/common/protobuf_utils.cpp f516091e9208552488c154f34adde6f20d3413bf > src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad > src/master/master.hpp 1fadbe61f774e8ca9926af4e39b3d6c8e24fe5df > src/master/master.cpp 9cea7bb6a6ee8bf5f4226d07111bcfa6f5d3a88c > > > Diff: https://reviews.apache.org/r/65591/diff/7/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
