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 37303: Moved scheduler library to http
Date Wed, 12 Aug 2015 05:09:37 GMT

-----------------------------------------------------------
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
> 
>


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