> On March 7, 2018, 3:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 7324 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line7326>
> >
> >     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;
> >         }
> >       }
> >     ```
> 
> Benjamin Bannier wrote:
>     It is possible on the proto level since e.g., 
> `RegisterSlaveMessage.resource_version_uuid` is `optional`. I'd argue that 
> the currently proposed implementation is more conservative in that it would 
> run the full handler even in the case you point out instead of aborting.
>     
>     I'd suggest we keep the impl like it is currently. Dropping.

What does it mean if `message.has_resource_version_uuid() && 
slave->resourceVersion.isNone()` is true? IIUC, that means that the agent 
previously registered with no resource version UUID for agent default 
resources, and it is now providing a resource version UUID in an 
UpdateSlaveMessage. Doesn't this indicate an error in the agent? Does it make 
sense to run the rest of the handler in such a case? I'm not sure what kind of 
precedent we have in the code for aborting the master on unexpected input from 
an agent, but if that's something we're in the habit of doing, this would seem 
like a suitable place for it.


> On March 7, 2018, 3:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Line 7722 (original), 7680-7681 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line7847>
> >
> >     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?
> 
> Benjamin Bannier wrote:
>     I do not think that is true. Only when the `resource_providers` field is 
> set does the agent send any resource provider information. The field could 
> contain information on zero or more resource providers in its `providers` 
> field, but we do not distinguish that here, exactly as you suggest.
>     
>     Dropping.

IIUC, the agent always sends info on all registered RPs in each 
UpdateSlaveMessage. So, if the master receives an UpdateSlaveMessage which 
indicates that no RPs are currently registered on an agent, doesn't that mean 
that offers for that agent with RP resources are currently invalid?


> On March 7, 2018, 3:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 10572-10574 (original), 10534-10536 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line10701>
> >
> >     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?
> 
> Benjamin Bannier wrote:
>     This is an important invariant of the current implementation, see 
> https://issues.apache.org/jira/browse/MESOS-8321.
>     
>     Dropping.

Are we sure that invariant holds in this case? The resource provider ID here is 
framework-supplied, and I don't think we validate that it's present in the 
master state before we hit this code.


- Greg


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65591/#review198741
-----------------------------------------------------------


On March 7, 2018, 11:36 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
> 
> (Updated March 7, 2018, 11:36 a.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 9c5fb97afb58f98013b79f3cbaea7dacc3603271 
>   src/master/http.cpp acf10a0f396234ec9f444c9b41515d657379d6ad 
>   src/master/master.hpp c9c8a968b6f56fe261ac9fe374e926a28d40fccb 
>   src/master/master.cpp e666b664dd125317cda5d16285d444b9c21e1f35 
> 
> 
> Diff: https://reviews.apache.org/r/65591/diff/8/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Reply via email to