mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <an...@apache.org>
Subject Re: Review Request 49301: Added FilesError class.
Date Tue, 28 Jun 2016 17:44:05 GMT

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



LGTM minus a few minor issues around style/code comments.


src/files/files.hpp (lines 53 - 55)
<https://reviews.apache.org/r/49301/#comment205176>

    hmm ..  `FilesError` can be used by more than master/agent actors. How about adding a
more generic comment on the use case of this error class e.g., something like:
    
    ```cpp
    // Represents the various errors that can be returned by methods on the `Files` class
via a `Try` that has failed.
    ```



src/files/files.hpp (line 61)
<https://reviews.apache.org/r/49301/#comment205177>

    Missing period at the end. Ditto for all the other 3 occurences.
    
    s/Request arguments are invalid/Invalid argument



src/files/files.hpp (line 62)
<https://reviews.apache.org/r/49301/#comment205178>

    s/could not be/not



src/files/files.hpp (line 71)
<https://reviews.apache.org/r/49301/#comment205164>

    `const std::string&`
    
    Also, no need for the `explicit` keyword on a multi arg constructor. Though, C++11 allows
you to use the brace initialization syntax, AFAICT, we don't use this widely in our code yet
and would be inconsistent.



src/files/files.hpp (lines 72 - 74)
<https://reviews.apache.org/r/49301/#comment205165>

    Should fit in one line, no?


- Anand Mazumdar


On June 28, 2016, 11:13 a.m., zhou xing wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49301/
> -----------------------------------------------------------
> 
> (Updated June 28, 2016, 11:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: mesos-5515
>     https://issues.apache.org/jira/browse/mesos-5515
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added FilesError class to identify the errors happened during
> the handling of files relevant requests. FilesError is a type
> deriving from Error that the master/agent can use to construct
> the response.
> 
> 
> Diffs
> -----
> 
>   src/files/files.hpp b767d5bc5bee16e3bd98199773a6bc7d30c1c32d 
> 
> Diff: https://reviews.apache.org/r/49301/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>


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