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 66545: Added admitted resource providers to the manager's registry.
Date Tue, 24 Apr 2018 19:29:31 GMT


> On April 23, 2018, 11:30 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 685 (patched)
> > <https://reviews.apache.org/r/66545/diff/6/?file=2009017#file2009017line685>
> >
> >     Let's move the actual logic from r/66546 to this patch to make it self-contained.
> >     
> >     Also, what's your opinion on having a different error message for re-subscribing
with an ID that has been removed?

I don't believe merging this patch with r/66546 is a good idea as they have different goals.
The patch here is also self-cointained in that it compiles and passes all tests. Dropping.

We don't currently (here or in r/66546) handle removed RP IDs at all. There also is currently
no way to remove an RP as the needed operator API calls do not exist, yet. Let's discuss what
to do about error messages when we implement that part.


> On April 23, 2018, 11:30 p.m., Chun-Hung Hsiao wrote:
> > src/resource_provider/manager.cpp
> > Lines 688 (patched)
> > <https://reviews.apache.org/r/66545/diff/6/?file=2009017#file2009017line688>
> >
> >     It's probably just my personal preference, but based on the experience working
on the codebase, i'd prefer `then` over `onAny` because the former gives the caller more control
on error handling, rather than relying on the callee checking states.
> >     
> >     Please feel free to drop this if you think `onAny` is a better construct here.

I agree slighlty :D

While `then` leads to nicer code on the happy path, I feel it requires a lot more code to
handle all the different failure scenarios then a simple check in an `onAny` handler (at least
when using `void` continutations). This is what I choose an `onAny` here.

Dropping.


- Benjamin


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


On April 23, 2018, 1:19 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66545/
> -----------------------------------------------------------
> 
> (Updated April 23, 2018, 1:19 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8402
>     https://issues.apache.org/jira/browse/MESOS-8402
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added admitted resource providers to the manager's registry.
> 
> 
> Diffs
> -----
> 
>   src/resource_provider/manager.cpp 68e1866986bfb91bf8355099ee1f0a2a86aba39a 
>   src/resource_provider/registrar.cpp 92ef9aecb1e93d10f46e53984391558f9901a60b 
>   src/resource_provider/registry.proto 14bd433ef056f8891e9a38153fdb06c39cee8f62 
>   src/tests/resource_provider_manager_tests.cpp c52541bf130ccf4795b989b53331176a64a097ea

> 
> 
> Diff: https://reviews.apache.org/r/66545/diff/7/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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