-----------------------------------------------------------
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
> 
>

Reply via email to