mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 38416: Allow HTTP response codes to be checked with code.
Date Tue, 06 Oct 2015 21:53:15 GMT

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



3rdparty/libprocess/include/process/http.hpp (lines 146 - 188)
<https://reviews.apache.org/r/38416/#comment159147>

    What is the value of this being an enum? We can't store the enum anywhere so every time
we want to use these we have to do explicit casting, like you have in Response below.
    
    How about making this a class called 'Status' which holds each of these as a static member
variable?
    
    That way we can still do:
    
    ```
    Status::OK
    ```
    
    And no explicit casting is necessary.



3rdparty/libprocess/include/process/http.hpp (lines 414 - 417)
<https://reviews.apache.org/r/38416/#comment159149>

    Rather than adding this to try to cope with the casting from the enum, let's avoid the
enum per my suggestion above and remove this helper.



3rdparty/libprocess/src/decoder.hpp (line 439)
<https://reviews.apache.org/r/38416/#comment159150>

    newline after this?



3rdparty/libprocess/src/decoder.hpp (line 441)
<https://reviews.apache.org/r/38416/#comment159151>

    Can you put this above the if, or just remove it?



3rdparty/libprocess/src/http.cpp (line 76)
<https://reviews.apache.org/r/38416/#comment159152>

    We avoid static non-POD types due to destruction issues. Can you put this on the heap?
    
    Per my suggestion above, perhaps we can have `Status::string(uint16_t code)` as a static
function on the `Status` class that provides the string version of the status.
    
    This should clean up the tests as well.


- Ben Mahler


On Oct. 6, 2015, 8:27 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38416/
> -----------------------------------------------------------
> 
> (Updated Oct. 6, 2015, 8:27 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, Jie Yu, Jojy Varghese, and Jiang
Yan Xu.
> 
> 
> Bugs: MESOS-3429
>     https://issues.apache.org/jira/browse/MESOS-3429
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allow HTTP response codes to be checked with code.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp 591c1a959057155e1bf0f5bd73352e78d1c15cb3

>   3rdparty/libprocess/src/decoder.hpp 53b8079968a0145651bad9d11aa4be2b504de57a 
>   3rdparty/libprocess/src/http.cpp ebf76093d654397260fc8da84c4f2b0fd6541483 
>   3rdparty/libprocess/src/process.cpp d1c81f1d244f02bf42cab97198587ce1b8c7c407 
>   3rdparty/libprocess/src/tests/benchmarks.cpp bb9ced8933bf2bb97ae6b3cffdb5528676e53c11

>   3rdparty/libprocess/src/tests/http_tests.cpp 38f3ad7e46f5b6ef4850cdf7fdcc115715e98472

>   3rdparty/libprocess/src/tests/process_tests.cpp ffd260a3fa2e49b3de183ba7b392b71afaaba2e5

> 
> Diff: https://reviews.apache.org/r/38416/diff/
> 
> 
> Testing
> -------
> 
> make
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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