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 54486: Added default values for all required IOSwitchboardServerFlags.
Date Sun, 11 Dec 2016 13:32:06 GMT


> On Dec. 8, 2016, 12:13 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/io/switchboard_main.cpp, lines 71-76
> > <https://reviews.apache.org/r/54486/diff/1/?file=1578815#file1578815line71>
> >
> >     Using sentinel values in the parameter domain as default values seems wrong
to me. If you want to detect explicitly given flags you should just use an `Option<T>`
for the parameter. This verification then becomes the semantically clearer
> >     
> >         if (flags.stdin_to_fd.isNone() ||
> >             ...
> >     
> >     With that no default value in the `add()` calls is required.
> >             
> >     You should probably also adjust the error message to something like `Missing
requireed parameters` and output `flags.usage()` after that.
> 
> Jie Yu wrote:
>     Sounds good.

@jieyu: I see this was committed without addressing any of these issues (or at least leaving
a comment), and also without updating the RR. Is there any plan to fix these?


- Benjamin


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


On Dec. 7, 2016, 6:06 p.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54486/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2016, 6:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-6726
>     https://issues.apache.org/jira/browse/MESOS-6726
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added default values for all required IOSwitchboardServerFlags.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/io/switchboard.hpp f691b182d4029a15bbb3b1c158b176d43f265cc1

>   src/slave/containerizer/mesos/io/switchboard_main.cpp 4a9ed22ca63f0aa55ebbf7b85404a1b168a8e34a

> 
> Diff: https://reviews.apache.org/r/54486/diff/
> 
> 
> Testing
> -------
> 
> GTEST_FILTER="" make -j check
> GTEST_FILTER="*IOSwitchboard*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>


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