mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 64477: Refactored agent to keep track of local resource providers.
Date Mon, 11 Dec 2017 11:24:46 GMT

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




src/resource_provider/manager.cpp
Lines 656-660 (original), 656-667 (patched)
<https://reviews.apache.org/r/64477/#comment271927>

    Nit: I'd prefer if we'd prepare a `hashmap` of operations before creating `updateState`
and `move` it into an arg in the aggregate initialization. That way we do not construct a
half-valid `updateState`.
    
        hashmap<UUID, OfferOperation> offerOperations;
        foreach (const OfferOperation &operation, update.operations()) {
          Try<UUID> uuid = UUID::fromBytes(operation.operation_uuid());
          CHECK_SOME(uuid);
        
          offerOperations.put(uuid.get(), operation);
        }
        
        ResourceProviderMessage::UpdateState updateState{
            resourceProvider->info,
            resourceVersion.get(),
            update.resources(),
            std::move(offerOperations)};



src/slave/slave.hpp
Lines 1072 (patched)
<https://reviews.apache.org/r/64477/#comment271931>

    We do not need this backpointer, so let's please remove it (IMO it is also not very good
design since it introduces too much coupling).



src/slave/slave.cpp
Lines 1548 (patched)
<https://reviews.apache.org/r/64477/#comment271923>

    Let's add a `CHECK` here that `info.has_id()`.



src/slave/slave.cpp
Lines 1569 (patched)
<https://reviews.apache.org/r/64477/#comment271924>

    Let's add a `CHECK` here that `info.has_id()`.



src/slave/slave.cpp
Line 2285 (original), 2299 (patched)
<https://reviews.apache.org/r/64477/#comment271929>

    This piece of code seems fragile under future changes even before this change since it
assumed that the agent _always_ knows as many resource providers as the master. I wonder if
we should explicitly allow for the agent to not know about a `resourceProvider` here instead
of crashing, e.g., remove the `CHECK_NOTNULL` but instead
    
        if (!resourceProvider ||
            resourceProvider->resourceVerdsion !=
            receivedResourceVersions.at(resourceProviderId.get())) {



src/slave/slave.cpp
Lines 7017-7019 (original)
<https://reviews.apache.org/r/64477/#comment271930>

    This information seemed like an important implementation detail. Why are we removing this
comment even though the approach has not changed?



src/slave/slave.cpp
Lines 7133 (patched)
<https://reviews.apache.org/r/64477/#comment271932>

    Not yours, but we should add
    
        CHECK(totalResources.contains(resourceProvider->totalResources));



src/slave/slave.cpp
Lines 7294 (patched)
<https://reviews.apache.org/r/64477/#comment271935>

    Since it is possible to e.g., `RESERVE` an empty `Resources`, I believe we could currently
trigger this `CHECK`.
    
    Even if we added master validation in the future it is still tricky to decide what status
update manager should keep track of reliably sending the update. I think it would make sense
to bookkeep this operation in a failed state to the agent so its status update manager can
in the future deliver the offer operation status to the framework.
    
    For now we could just drop operations without an extractable resource provider id here
with some logging and a `TODO`.



src/slave/slave.cpp
Lines 7412 (patched)
<https://reviews.apache.org/r/64477/#comment271936>

    See comment on `Slave::addOfferOperation`.



src/slave/slave.cpp
Lines 7444 (patched)
<https://reviews.apache.org/r/64477/#comment271925>

    Let's add a `CHECK` here that `info.has_id()`.


- Benjamin Bannier


On Dec. 10, 2017, 6:11 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64477/
> -----------------------------------------------------------
> 
> (Updated Dec. 10, 2017, 6:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Currently, we don't explicitly keep track of local resources providers.
> This causes the logic for a few methods to be quite complex because we
> need to reconstruct the resource provider information everytime.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/manager.cpp f98611c351f11b15281dd3ef673ef80db1c44f07 
>   src/resource_provider/message.hpp bbf6bb2a0a4aa901ac038cdbe4dd46acc0f2e453 
>   src/slave/http.cpp 738786fc4b85903f187cf0988a3fca488ea2767d 
>   src/slave/slave.hpp b3a1e70169ed1abbfc815821771715197a63f2df 
>   src/slave/slave.cpp 373e393ca1e7c0c30c3474cc9e630e25ad92f235 
>   src/tests/resource_provider_manager_tests.cpp a6eb4c9a303780029244e069bdf550a8cd9c7bb4

> 
> 
> Diff: https://reviews.apache.org/r/64477/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message