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 Thu, 08 Dec 2016 11:13:23 GMT

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




src/slave/containerizer/mesos/io/switchboard_main.cpp (lines 71 - 76)
<https://reviews.apache.org/r/54486/#comment229281>

    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.



src/slave/containerizer/mesos/io/switchboard_main.cpp (line 79)
<https://reviews.apache.org/r/54486/#comment229282>

    Unrelated to this patch, but this executable should probably also output usage information
if invoked with `--help` (sadly this isn't handled on the `flags::FlagsBase` level). The canonical
form is
    
        if (flags.help) {
          cerr << flags.usage();
          return EXIT_SUCCESS;
        }


- Benjamin Bannier


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