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


Thanks Anand, mostly thinking we can clean up the read logic if we have a 
struct to capture the reader / decoder.


src/common/http.hpp (lines 51 - 53)
<https://reviews.apache.org/r/37303/#comment149809>

    Can't we just have support for stringifying these?
    
    e.g. stringify(ContentType::PROTOBUF) == "APPLICATION_PROTOBUF"
    
    Or is there something I'm missing?



src/common/http.hpp (lines 80 - 81)
<https://reviews.apache.org/r/37303/#comment149808>

    value.get() need to be called only if value.isSome()



src/scheduler/scheduler.cpp (line 111)
<https://reviews.apache.org/r/37303/#comment149810>

    This is just for testing right? Might be nice to expose as a separate 
constructor with a note that it's for testing.



src/scheduler/scheduler.cpp (line 117)
<https://reviews.apache.org/r/37303/#comment149812>

    Hm.. can we avoid the 'headers' variable? Seems clearer to keep it local to 
an individual request for now.



src/scheduler/scheduler.cpp (lines 219 - 221)
<https://reviews.apache.org/r/37303/#comment149817>

    This is required, so how about just saying:
    
    ```
    // Each subscription requires a new connection.
    disconnect();
    ```



src/scheduler/scheduler.cpp (lines 222 - 223)
<https://reviews.apache.org/r/37303/#comment149816>

    Can we call this `disconnect`? Any reason to not clear the reader within 
the function rather than in the caller?



src/scheduler/scheduler.cpp (line 323)
<https://reviews.apache.org/r/37303/#comment149824>

    Need to deal with isFailed case before you call .get() as well.



src/scheduler/scheduler.cpp (line 325)
<https://reviews.apache.org/r/37303/#comment149820>

    Looks like you don't need this, how about:
    
    if (call.type() == Call::SUBSCRIBE &&
        response.get().status == process::http::statuses[200]) {
      ...   
    } else if (response.get().status == process::http::statuses[202]) {
      ...
    } else {
      error!
    }



src/scheduler/scheduler.cpp (line 328)
<https://reviews.apache.org/r/37303/#comment149818>

    Can you flip this comparison?



src/scheduler/scheduler.cpp (line 329)
<https://reviews.apache.org/r/37303/#comment149819>

    CHECK_EQ?



src/scheduler/scheduler.cpp (lines 332 - 338)
<https://reviews.apache.org/r/37303/#comment149822>

    Seems like having a higher level struct, like we did with HttpConnection in 
the master, will help us here as well?
    
    That can provide a 'read' function that returns a Future<Event>. When we 
process the read, we can see if the connection is still the same in order to 
decide whether to continue reading.



src/scheduler/scheduler.cpp (lines 353 - 355)
<https://reviews.apache.org/r/37303/#comment149821>

    How about:
    
    ```
    Received a '500 Internal Error' for SUBSCRIBE call: Body of request
    ```
    
    Remember that we don't use periods in logging messages



src/scheduler/scheduler.cpp (line 383)
<https://reviews.apache.org/r/37303/#comment149826>

    Failed to decode the stream of events: ...



src/scheduler/scheduler.cpp (lines 390 - 397)
<https://reviews.apache.org/r/37303/#comment149825>

    Why does master disconnection send an Error event? This can occur if the 
master crashes, fails over, etc. So the scheduler should not see an error for 
this?



src/scheduler/scheduler.cpp (lines 400 - 401)
<https://reviews.apache.org/r/37303/#comment149827>

    s/chunk/event/
    
    How about:
    
    ```
    Failed to de-serialize event sent by master: ...
    ```



src/scheduler/scheduler.cpp (line 435)
<https://reviews.apache.org/r/37303/#comment149813>

    Can we call this 'disconnect'?



src/scheduler/scheduler.cpp (line 437)
<https://reviews.apache.org/r/37303/#comment149814>

    space here :)



src/scheduler/scheduler.cpp (line 439)
<https://reviews.apache.org/r/37303/#comment149815>

    Can we avoid saying "reader" in these messages? Let's just say that the 
connection was already closed, since the users of this library don't care about 
the implementation detail of there being a Reader.


- Ben Mahler


On Aug. 11, 2015, 10:18 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37303/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2015, 10:18 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-2552
>     https://issues.apache.org/jira/browse/MESOS-2552
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This changes moves scheduler library to http. The change to remove auth + 
> install,receive handlers are in other reviews.
> 
> 
> Diffs
> -----
> 
>   src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838 
>   src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50 
>   src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 
> 
> Diff: https://reviews.apache.org/r/37303/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Reply via email to