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


Sorry for not elaborating on all of these, I added some more explanations here. 
Main thing is cleaning up the read loop and figuring out the callback semantics 
(do we need to revisit 'connected' / 'disconnected'?).


src/common/http.hpp (lines 56 - 57)
<https://reviews.apache.org/r/37303/#comment149859>

    You can `return stream << ...;` in a single statement.



src/common/http.hpp (lines 97 - 99)
<https://reviews.apache.org/r/37303/#comment149860>

    Feel free to just return directly, rather than storing in a local variable.



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

    Sorry, let me elaborate further. I mentioned not having this because 
"headers" requires non-local reasoning when reading the request sending code:
    
    ```
          // Send a streaming request for Subscribe call.
          response = process::http::streaming::post(
              master.get(),
              "api/v1/scheduler",
              headers, // non-local
              body,
              stringify(contentType));
              
    vs.
    
          // Send a streaming request for Subscribe call.
          response = process::http::streaming::post(
              master.get(),
              "api/v1/scheduler",
              {{"Accept", stringify(contentType)}}, // local
              body,
              stringify(contentType));
    ```
    
    We've generally found local reasoning makes code easier to read (a.k.a. 
"readable"). Also, keep in mind that in the future you'll be able to send a 
Request directly, so it would become:
    
    ```
    Request request;
    ...
    request.headers["Accept"] = stringify(contentType);
    
    response = process::http::streaming::request(request);
    ```
    
    The other concern is that headers is not necessarily going to remain 
constant like this in the future (e.g. if we add the notion of a session 
header). Make sense?



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

    remove the space here



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

    The reason I ask to flip this is we've generally not used "yoda" style 
comparisons, e.g.
    
    ```
    size() > 1
    
    rather than
    
    1 < size()
    ```
    
    Can you do a sweep in the code you've introduced here?



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

    Flip the comparison here as well. Be sure to do a sweep.



src/scheduler/scheduler.cpp (lines 354 - 362)
<https://reviews.apache.org/r/37303/#comment149868>

    I suggested the struct because passing around the recodio reader 
independently of the pipe reader seems to be not that intuitive, was hoping to 
see them stored together inside an Option<Connection> rather than having the 
Option<Pipe::Reader> and recordio::Reader stored separately. Make more sense?
    
    Also, can we use recordio::Reader to be more explicit about this type?



src/scheduler/scheduler.cpp (lines 387 - 394)
<https://reviews.apache.org/r/37303/#comment149865>

    Ok, let's chat about this what to do here, might need to revisit the 
callbacks here now that we have http. But this shouldn't be an error since it's 
just disconnection. If we call disconnected here though, seems that we need to 
immediately call connected if there is still a master available, otherwise we 
might hang around when we should be retrying.



src/scheduler/scheduler.cpp (lines 388 - 389)
<https://reviews.apache.org/r/37303/#comment149866>

    There is no "end of file chunk".. :)


- Ben Mahler


On Aug. 12, 2015, 4:12 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37303/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2015, 4:12 a.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/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316 
> 
> Diff: https://reviews.apache.org/r/37303/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Reply via email to