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 64932: Added example framework converting disk resources.
Date Fri, 05 Jan 2018 12:38:07 GMT


> On Jan. 4, 2018, 9:49 p.m., Gaston Kleiman wrote:
> > src/examples/test_csi_user_framework.cpp
> > Lines 278 (patched)
> > <https://reviews.apache.org/r/64932/diff/1/?file=1930012#file1930012line278>
> >
> >     I'd rephrase this:
> >     
> >     ```
> >     Check whether the given resources are reserved, ensuring that all RAW/MOUNT
disk resources offered by the resource provider are either reserved or unreserved.
> >     ```
> >     
> >     Wouldn't this break if a resource provider offers both reserved and unreserved
disk resources? I guess this will be usual once the agent's default disk resources (non-csi)
are handled by an SLRP.

Fixed, see comment below, https://reviews.apache.org/r/64932/#comment273888.


> On Jan. 4, 2018, 9:49 p.m., Gaston Kleiman wrote:
> > src/examples/test_csi_user_framework.cpp
> > Lines 279 (patched)
> > <https://reviews.apache.org/r/64932/diff/1/?file=1930012#file1930012line279>
> >
> >     Why do you use an option here?
> >     
> >     Wouldn't `CHECK(!resources.isEmpty())` be clearer?

This piece checked whether all resources handled here are either _all reserved_ or that _none
is reserved_. I am not sure the alternative you proposed is equivalent.

I moved the checked of reservedness to the previous step so this issue does not apply anymore;
dropping.


> On Jan. 4, 2018, 9:49 p.m., Gaston Kleiman wrote:
> > src/examples/test_csi_user_framework.cpp
> > Lines 302 (patched)
> > <https://reviews.apache.org/r/64932/diff/1/?file=1930012#file1930012line302>
> >
> >     Use `endl` instead of `'\n'`.

Adjusted here and elsewhere for consistency.


> On Jan. 4, 2018, 9:49 p.m., Gaston Kleiman wrote:
> > src/examples/test_csi_user_framework.cpp
> > Lines 331-332 (patched)
> > <https://reviews.apache.org/r/64932/diff/1/?file=1930012#file1930012line331>
> >
> >     I'd phrase it:
> >     
> >     If we didn't create an `ACCEPT` operation, create a `DELCINE` operation.
> 
> Greg Mann wrote:
>     +1

We might still create an `ACCEPT` but add no operations in which case we would like to decline
as well. I went with

    // If we did not create operations to accept the offer with decline it.


> On Jan. 4, 2018, 9:49 p.m., Gaston Kleiman wrote:
> > src/examples/test_csi_user_framework.cpp
> > Lines 380-386 (patched)
> > <https://reviews.apache.org/r/64932/diff/1/?file=1930012#file1930012line380>
> >
> >     This method doesn't seem to be used. `int main(...)` should probably call `Flags::setUsageMessage`
instead of this.
> 
> Greg Mann wrote:
>     Yea weird, we have these in a couple other example frameworks as well. Should probably
clean them up when we have a chance.

I removed the function here.


> On Jan. 4, 2018, 9:49 p.m., Gaston Kleiman wrote:
> > src/examples/test_csi_user_framework.cpp
> > Lines 399 (patched)
> > <https://reviews.apache.org/r/64932/diff/1/?file=1930012#file1930012line399>
> >
> >     Why don't we make this a `string` instead of an `Option<string>`?
> >     
> >     That way we could remove the: `if (flags.master.isNone())` block in the main
function.

That's a great suggestion. The current code here does follow what virtually every other example
framework does though.


> On Jan. 4, 2018, 9:49 p.m., Gaston Kleiman wrote:
> > src/examples/test_csi_user_framework.cpp
> > Lines 444-447 (patched)
> > <https://reviews.apache.org/r/64932/diff/1/?file=1930012#file1930012line444>
> >
> >     Shouldn't this be a flag like in `no_executor_framework.cpp` and in `long_lived_framework.cpp`?

We do appear to be inconsistent here, e.g., `load_generator_framework.cpp`, `test_framework.cpp`,
or `test_http_framework.cpp` use an environment variable.

I'd prefer to keep this as is and leave it for a broader cleanup which could e.g., make it
possible to load flags from both environment or command line flags; dropping.


- Benjamin


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


On Jan. 5, 2018, 1:37 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64932/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2018, 1:37 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch introduces an example HTTP framework which transforms
> 'RAW' disk resources from resource providers into 'MOUNT' volumes and
> subsequently unreserves them.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 30cd4d426e797e4c8ee556d1bc3de99830a5fe41 
>   src/examples/CMakeLists.txt d4f1af4f072efdc68fa0b722f42b1d8aa1779b6e 
>   src/examples/test_csi_user_framework.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64932/diff/2/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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