> On March 7, 2018, 4: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;
> >         }
> >       }
> >     ```

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.


> On March 7, 2018, 4: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?

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.


> On March 7, 2018, 4: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?

This is an important invariant of the current implementation, see 
https://issues.apache.org/jira/browse/MESOS-8321.

Dropping.


> On March 7, 2018, 4:13 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 11515 (patched)
> > <https://reviews.apache.org/r/65591/diff/7/?file=1971488#file1971488line11686>
> >
> >     Ditto with this CHECK - are we sure that a well-formed but unknown RP 
> > ID wouldn't hit this?

Ditto, see https://issues.apache.org/jira/browse/MESOS-8321.

Dropping.


- Benjamin


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


On March 7, 2018, 12:36 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65591/
> -----------------------------------------------------------
> 
> (Updated March 7, 2018, 12:36 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 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