You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <ma...@gmail.com> on 2015/08/10 18:43:36 UTC
Review Request 37303: Moved scheduler library to http
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/
-----------------------------------------------------------
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 a8699a7514892a548eb03f9dcb6a1198ce518830
Diff: https://reviews.apache.org/r/37303/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
> > src/common/http.hpp, line 73
> > <https://reviews.apache.org/r/37303/diff/1/?file=1036466#file1036466line73>
> >
> > this helper seems weird. i would rather have isOK()and isAccepted() helpers.
> >
> > anyway, this seems to be used only once. do we even need a helper?
Removed method.
> On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
> > src/common/http.cpp, line 86
> > <https://reviews.apache.org/r/37303/diff/1/?file=1036467#file1036467line86>
> >
> > add a default case please, in case someone adds a new supported contentType enum.
Why ? I want the compilation to fail when a user adds a new supported contentType enum ( -WSwitch )
> On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
> > src/common/http.cpp, line 101
> > <https://reviews.apache.org/r/37303/diff/1/?file=1036467#file1036467line101>
> >
> > ditto. add a default case.
Same as above.
> On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 354
> > <https://reviews.apache.org/r/37303/diff/1/?file=1036468#file1036468line354>
> >
> > you should make sure subscribe got an OK and everything else got Accepted. for that this method needs to take Call as argument.
Logging an error now for such cases.
> On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, lines 426-427
> > <https://reviews.apache.org/r/37303/diff/1/?file=1036468#file1036468line426>
> >
> > When does this happen?
This can happen when there might be some corruption of data across the wire, or the server sends a in-complete protobuf/json response. Added the error from decoder to the string too.
> On Aug. 10, 2015, 7:52 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 415
> > <https://reviews.apache.org/r/37303/diff/1/?file=1036468#file1036468line415>
> >
> > When does this happen? Add a comment.
> >
> > Also, sending a "subscribe with master.." with every error event seems a bit redundat? Can we just add documentation to the ERROR event in the protobuf saying so? Or are there events where we dont want schedulers to oversubscribe?
Added.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/#review94777
-----------------------------------------------------------
On Aug. 11, 2015, 12:17 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37303/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 12:17 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/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50
> src/scheduler/scheduler.cpp a8699a7514892a548eb03f9dcb6a1198ce518830
>
> Diff: https://reviews.apache.org/r/37303/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/#review94777
-----------------------------------------------------------
looks like the SchedulerTest.Subscribe test failed?
src/common/http.hpp (line 66)
<https://reviews.apache.org/r/37303/#comment149373>
// Deserializes a string message into a protobuf message
// based on the HTTP content type
src/common/http.hpp (line 67)
<https://reviews.apache.org/r/37303/#comment149378>
hmm. this is not as generic as serialize. can't this be templated?
template <typename T>
Try<T> deserialize(...);
src/common/http.hpp (line 73)
<https://reviews.apache.org/r/37303/#comment149375>
this helper seems weird. i would rather have isOK()and isAccepted() helpers.
anyway, this seems to be used only once. do we even need a helper?
src/common/http.cpp (line 86)
<https://reviews.apache.org/r/37303/#comment149379>
add a default case please, in case someone adds a new supported contentType enum.
src/common/http.cpp (line 101)
<https://reviews.apache.org/r/37303/#comment149380>
ditto. add a default case.
src/scheduler/scheduler.cpp (line 211)
<https://reviews.apache.org/r/37303/#comment149382>
why new synchronous function (_send()) here instead of directly plopping the code here?
we prefer procedural programming style, i.e., we don't introduce new methods/functions unless it's necesary.
src/scheduler/scheduler.cpp
<https://reviews.apache.org/r/37303/#comment149383>
this should've been in the patch that removed pid related stuff.
src/scheduler/scheduler.cpp (line 296)
<https://reviews.apache.org/r/37303/#comment149384>
s/call type Subscribe/Subscribe call/
src/scheduler/scheduler.cpp (line 314)
<https://reviews.apache.org/r/37303/#comment149386>
```
s/handleResponse/_send/
```
src/scheduler/scheduler.cpp (line 315)
<https://reviews.apache.org/r/37303/#comment149388>
handling error like this loses the context about the call that caused the error, no?
i would just do a .onAny() instead of .then() and .onFailed().
src/scheduler/scheduler.cpp (line 320)
<https://reviews.apache.org/r/37303/#comment149385>
you should make sure subscribe got an OK and everything else got Accepted. for that this method needs to take Call as argument.
src/scheduler/scheduler.cpp (line 322)
<https://reviews.apache.org/r/37303/#comment149389>
ditto. this should include which call/request failed.
src/scheduler/scheduler.cpp (line 334)
<https://reviews.apache.org/r/37303/#comment149392>
This needs a comment. Why would there be an existing reader? Why is an existing reader not closed as soon as a subscribe call is made?
src/scheduler/scheduler.cpp (line 345)
<https://reviews.apache.org/r/37303/#comment149390>
are all other response types (non PIP) ok to receive?
src/scheduler/scheduler.cpp (lines 358 - 359)
<https://reviews.apache.org/r/37303/#comment149393>
Just say that Events stream filed and include the failure message.
src/scheduler/scheduler.cpp (line 375)
<https://reviews.apache.org/r/37303/#comment149394>
ditto. please don't create new functions unless there is re-use.
src/scheduler/scheduler.cpp (line 381)
<https://reviews.apache.org/r/37303/#comment149396>
When does this happen? Add a comment.
Also, sending a "subscribe with master.." with every error event seems a bit redundat? Can we just add documentation to the ERROR event in the protobuf saying so? Or are there events where we dont want schedulers to oversubscribe?
src/scheduler/scheduler.cpp (lines 392 - 393)
<https://reviews.apache.org/r/37303/#comment149397>
When does this happen?
src/scheduler/scheduler.cpp (lines 406 - 407)
<https://reviews.apache.org/r/37303/#comment149400>
include the error please.
src/scheduler/scheduler.cpp (line 423)
<https://reviews.apache.org/r/37303/#comment149401>
make this a LOG(WARNING)
src/scheduler/scheduler.cpp (lines 483 - 490)
<https://reviews.apache.org/r/37303/#comment149387>
```
process = new MesosProcess(
arg1,
arg2,
arg3,
arg4,
arg5);
```
- Vinod Kone
On Aug. 10, 2015, 4:43 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37303/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 4:43 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 a8699a7514892a548eb03f9dcb6a1198ce518830
>
> Diff: https://reviews.apache.org/r/37303/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
> 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
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/#review95226
-----------------------------------------------------------
src/common/http.hpp (line 51)
<https://reviews.apache.org/r/37303/#comment150063>
kill this.
src/scheduler/scheduler.cpp (line 257)
<https://reviews.apache.org/r/37303/#comment150077>
make sure to close the http connection here to avoid accidentally sending non-subscribe calls to a new master.
src/scheduler/scheduler.cpp (line 321)
<https://reviews.apache.org/r/37303/#comment150064>
add a comment on when this is possible.
src/scheduler/scheduler.cpp (line 342)
<https://reviews.apache.org/r/37303/#comment150066>
s/keepReading/read/
src/scheduler/scheduler.cpp (line 351)
<https://reviews.apache.org/r/37303/#comment150067>
s/a/unexpected/
src/scheduler/scheduler.cpp (line 353)
<https://reviews.apache.org/r/37303/#comment150074>
when can we get here? can you add a comment?
per our disccussion, this should call error()?
src/scheduler/scheduler.cpp (line 359)
<https://reviews.apache.org/r/37303/#comment150078>
indentation.
src/scheduler/scheduler.cpp (line 365)
<https://reviews.apache.org/r/37303/#comment150068>
s/oldReader/reader/
src/scheduler/scheduler.cpp (line 383)
<https://reviews.apache.org/r/37303/#comment150069>
this could happen during master failover too. so let's not send an error()
src/scheduler/scheduler.cpp (line 396)
<https://reviews.apache.org/r/37303/#comment150071>
indent by 2 spaces.
s/WARNING/ERROR/
this should call error().
- Vinod Kone
On Aug. 12, 2015, 10:26 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37303/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2015, 10:26 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/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316
>
> Diff: https://reviews.apache.org/r/37303/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/#review95240
-----------------------------------------------------------
Ship it!
Looks good modulo minor fixes. I'll fix them and commit for you.
src/scheduler/scheduler.cpp (line 329)
<https://reviews.apache.org/r/37303/#comment150090>
indentation.
src/scheduler/scheduler.cpp (line 393)
<https://reviews.apache.org/r/37303/#comment150091>
No comment here?
src/scheduler/scheduler.cpp (line 412)
<https://reviews.apache.org/r/37303/#comment150092>
indentation.
- Vinod Kone
On Aug. 13, 2015, 5:39 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37303/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 5:39 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
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/
-----------------------------------------------------------
(Updated Aug. 13, 2015, 5:39 a.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Review comments from Vinod
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 (updated)
-----
src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838
src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316
Diff: https://reviews.apache.org/r/37303/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/
-----------------------------------------------------------
(Updated Aug. 12, 2015, 10:26 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Rebased.
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 (updated)
-----
src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838
src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316
Diff: https://reviews.apache.org/r/37303/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Moved Pipe::reader, decoder to a separate struct Connection + Other review comments from Ben
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 (updated)
-----
src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838
src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316
Diff: https://reviews.apache.org/r/37303/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Review Comments from Ben
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 (updated)
-----
src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838
src/scheduler/scheduler.cpp 3fbe3831cf93c29180bc7e433fd57c6108988316
Diff: https://reviews.apache.org/r/37303/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > Thanks Anand, mostly thinking we can clean up the read logic if we have a struct to capture the reader / decoder.
Isn't it much more simpler here? It's just a one liner "if" check to check if the reader is reader is not None and != for stale reader check. Here is the code snipped I have used:
if (!reader.isSome() || reader.get() != oldReader) {
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, lines 353-355
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line353>
> >
> > How about:
> >
> > ```
> > Received a '500 Internal Error' for SUBSCRIBE call: Body of request
> > ```
> >
> > Remember that we don't use periods in logging messages
Sounds good to me. I suppose we don't use periods to terminate logging messages ?
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, lines 390-397
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line390>
> >
> > 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?
How else would you communicate to the scheduler that it needs to subscribe again in case of master disconnection/failover ? Feel free to re-open in case I missed something
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 328
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line328>
> >
> > Can you flip this comparison?
Why would you want to do that ? process::http::statuses[200] is a constant and helps identify bugs like if (a = 5) by keeping the constant check on the left ? Seems like I am missing something here
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 111
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line111>
> >
> > This is just for testing right? Might be nice to expose as a separate constructor with a note that it's for testing.
Not quite. I intend to add an constructor to Mesos::Mesos i.e. the wrapper exposed class to the user later that would be used for testing. This is just the internal implementation class.
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 117
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line117>
> >
> > Hm.. can we avoid the 'headers' variable? Seems clearer to keep it local to an individual request for now.
Why ? This is anyways just the internal implementation class. Since we have to do that for every request we make. Why not just save it as a constant member variable ?
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, lines 219-221
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line219>
> >
> > This is required, so how about just saying:
> >
> > ```
> > // Each subscription requires a new connection.
> > disconnect();
> > ```
Sounds good. Done
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/common/http.hpp, lines 80-81
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037897#file1037897line80>
> >
> > value.get() need to be called only if value.isSome()
Fixed , my bad.
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, lines 222-223
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line222>
> >
> > Can we call this `disconnect`? Any reason to not clear the reader within the function rather than in the caller?
Fixed
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 323
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line323>
> >
> > Need to deal with isFailed case before you call .get() as well.
Fixed
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 329
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line329>
> >
> > CHECK_EQ?
Fixed
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 435
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line435>
> >
> > Can we call this 'disconnect'?
Fixed
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 437
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line437>
> >
> > space here :)
I wish we as a community can get adoption for clang format soon. These things should be handled by a tool rather then someone spending time reviewing them :(
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 439
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line439>
> >
> > 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.
Fixed
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, line 383
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line383>
> >
> > Failed to decode the stream of events: ...
Fixed
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/scheduler/scheduler.cpp, lines 400-401
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037899#file1037899line400>
> >
> > s/chunk/event/
> >
> > How about:
> >
> > ```
> > Failed to de-serialize event sent by master: ...
> > ```
Fixed
> On Aug. 12, 2015, 1:20 a.m., Ben Mahler wrote:
> > src/common/http.hpp, lines 51-53
> > <https://reviews.apache.org/r/37303/diff/3/?file=1037897#file1037897line51>
> >
> > Can't we just have support for stringifying these?
> >
> > e.g. stringify(ContentType::PROTOBUF) == "APPLICATION_PROTOBUF"
> >
> > Or is there something I'm missing?
Good point. Added.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/#review95036
-----------------------------------------------------------
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
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Updated to using RecordIO reader now. Also setting reader to none on receiving another subscribe event.
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 (updated)
-----
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
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Aug. 11, 2015, 6:55 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 329
> > <https://reviews.apache.org/r/37303/diff/2/?file=1036937#file1036937line329>
> >
> > indent by 4 spaces.
Not used now. Fixed the other one.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/#review94959
-----------------------------------------------------------
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
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/#review94959
-----------------------------------------------------------
src/scheduler/scheduler.cpp (lines 328 - 332)
<https://reviews.apache.org/r/37303/#comment149702>
can you use the response decoder here?
src/scheduler/scheduler.cpp (line 329)
<https://reviews.apache.org/r/37303/#comment149689>
indent by 4 spaces.
src/scheduler/scheduler.cpp (line 330)
<https://reviews.apache.org/r/37303/#comment149690>
indent by 4 spaces.
src/scheduler/scheduler.cpp (line 346)
<https://reviews.apache.org/r/37303/#comment149691>
also print the response.get().status?
src/scheduler/scheduler.cpp (line 436)
<https://reviews.apache.org/r/37303/#comment149694>
why is this method private but most others are protected?
src/scheduler/scheduler.cpp (line 447)
<https://reviews.apache.org/r/37303/#comment149693>
const?
src/scheduler/scheduler.cpp (line 475)
<https://reviews.apache.org/r/37303/#comment149685>
as discussed, please make this a paramter to Mesos() constructor. add a TODO for now and follow up in a different patch.
- Vinod Kone
On Aug. 11, 2015, 12:17 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37303/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 12:17 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/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50
> src/scheduler/scheduler.cpp a8699a7514892a548eb03f9dcb6a1198ce518830
>
> Diff: https://reviews.apache.org/r/37303/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 37303: Moved scheduler library to http
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37303/
-----------------------------------------------------------
(Updated Aug. 11, 2015, 12:17 a.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Vinod's review comments.
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 (updated)
-----
src/common/http.hpp 98a12709ae5fcbf4ce3e37d164bff43818096838
src/common/http.cpp e2ff48ca609f1c97a1b6c20cc6be3d97c3e4bb50
src/scheduler/scheduler.cpp a8699a7514892a548eb03f9dcb6a1198ce518830
Diff: https://reviews.apache.org/r/37303/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar