mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anand Mazumdar" <mazumdar.an...@gmail.com>
Subject Re: Review Request 37303: Moved scheduler library to http
Date Wed, 12 Aug 2015 06:52:12 GMT


> On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
> > 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'?).

Let's keep the callback behavior the same as what it was before. We can decide to change the
semantics if we feel the need later in a separate patch.


> On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 117
> > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line117>
> >
> >     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?

Anyways , this won't compile. I added it as a const local variable for now.


> On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
> > src/common/http.hpp, lines 56-57
> > <https://reviews.apache.org/r/37303/diff/4/?file=1038062#file1038062line56>
> >
> >     You can `return stream << ...;` in a single statement.

Looked more readable this way. Made the change though.


> On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 329
> > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line329>
> >
> >     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?

Fixed. I guess we already have -Wall,-Werror in our code. So my concerns around accidently
assigning in "if" statements don't make much sense here.


> On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 330
> > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line330>
> >
> >     Flip the comparison here as well. Be sure to do a sweep.

Done. CHECK_EQ though normally have syntax like CHECK_EQ(expected, actual)


> On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, lines 354-362
> > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line354>
> >
> >     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?

Done, Moved it to a struct Connection. Also since RecordIO::Reader is a process now. Saving
it in a process::Owned member field inside the struct.


> On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, lines 387-394
> > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line387>
> >
> >     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.

Removed the error event. Just logging an error for now.


> On Aug. 12, 2015, 5:09 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, lines 388-389
> > <https://reviews.apache.org/r/37303/diff/4/?file=1038063#file1038063line388>
> >
> >     There is no "end of file chunk".. :)

Fixed, My bad :) Even end of file event hardly made any sense , made it "End-Of-File received
from master. The master closed the event stream"


- Anand


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


On Aug. 12, 2015, 6:51 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, 6:51 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