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

Reply via email to