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 49446: Implemented LIST_FILES Call in v1 master API.
Date Thu, 30 Jun 2016 22:21:34 GMT

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




include/mesos/master/master.proto (line 110)
<https://reviews.apache.org/r/49446/#comment205580>

    Let's be more particular that is the file listing for a directory.
    
    How about:
    
    ```cpp
    // Provides the file listing for a directory.
    ```
    
    Ditto for the versioned protobuf.



include/mesos/master/master.proto (line 273)
<https://reviews.apache.org/r/49446/#comment205594>

    Can we include a comment here about the order guarantees provided by the response i.e.
they are sorted on the `path`?



src/master/http.cpp (line 2789)
<https://reviews.apache.org/r/49446/#comment205581>

    Kill this. We don't have such a check for the other handler functions? Would be good to
be consistent all around.



src/master/http.cpp (line 2791)
<https://reviews.apache.org/r/49446/#comment205582>

    Nit: Capture by const reference for an alias.



src/master/http.cpp (line 2793)
<https://reviews.apache.org/r/49446/#comment205589>

    Can we just capture `contentType` in the lambda?
    
    Also, in the future if someone adds something in the lambda accesses the `this` pointer
i.e. some member in the master actor. This would lead to a potential race condition.
    
    Can we enclose this lambda in a `defer()`?
    
    i.e. then(defer([contentType](....){})



src/master/http.cpp (line 2794)
<https://reviews.apache.org/r/49446/#comment205590>

    `const Try<T>&`.
    
    Also, see earlier review comment about avoiding the `map` return type and just returning
the list of files.



src/master/http.cpp (line 2804)
<https://reviews.apache.org/r/49446/#comment205587>

    Include the `error.message` as part of the `Forbidden` response?



src/master/http.cpp (line 2807)
<https://reviews.apache.org/r/49446/#comment205585>

    Can we include `error.message` too as part of `NotFound()` response body?



src/master/http.cpp (lines 2809 - 2810)
<https://reviews.apache.org/r/49446/#comment205584>

    See comments in earlier review around avoiding `default`



src/master/http.cpp (line 2813)
<https://reviews.apache.org/r/49446/#comment205592>

    Can we have an explicit defensive check here to ensure `result` does not have error set?
    
    Also, applies to the prior review for `/files/browse`.



src/master/http.cpp (line 2816)
<https://reviews.apache.org/r/49446/#comment205583>

    Nit: New line before.


- Anand Mazumdar


On June 30, 2016, 2:06 p.m., Abhishek Dasgupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49446/
> -----------------------------------------------------------
> 
> (Updated June 30, 2016, 2:06 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5487
>     https://issues.apache.org/jira/browse/MESOS-5487
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented LIST_FILES Call in v1 master API.
> 
> 
> Diffs
> -----
> 
>   include/mesos/master/master.proto 2e5d6eeb3a960e4f41c382d65321f18bb05ed6be 
>   include/mesos/v1/master/master.proto 93157d57dcc53b54fed2ebbc4772c689ddba2119 
>   src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 
>   src/master/master.hpp e2ab2110fe5a287ab16ac9ef4222fed633e02ebe 
> 
> Diff: https://reviews.apache.org/r/49446/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>


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