Re: Review Request 36318: Refactored framework struct in master to support http frameworks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/#review93331 --- Ship it! src/master/master.cpp (line 4841) https://reviews.apache.org/r/36318/#comment147708 This needs to go up before we try to send a message (or SchedulerDriverEventTest.SubscribedSchedulerFailover fails). To be safe, I'll also remove the logic to drop when !connected, and just log a warning instead. - Ben Mahler On July 28, 2015, 3:47 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 28, 2015, 3:47 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change refactors the framework struct in master to introduce support for http frameworks. - pid becomes a optional field now in the framework struct. - added optional fields for supporting http frameworks to the framework struct Diffs - src/common/http.hpp 765860fa7d0ce354320e9d293d09719be87efca0 src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070 src/master/master.hpp 2c924addfb4c52d3048ee6ded13ce638145cc93f src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510 Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + tests now go in a separate patch now. Thanks, Anand Mazumdar
Re: Review Request 36318: Refactored framework struct in master to support http frameworks
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/#review93165
---
Nice work Anand!
I left feedback here, but it is all addressed in the diff I sent you over
email. With the diff applied this patch looks like a shippable step to me. Note
that per the comments, I also fenced off addFramework for http schedulers (much
like you've already done here for failoverFramework). In both of these, we'll
need to set up a readerClosed callback (the equivalent of link()). I also
noticed that we'll need connection equality for this, so I'll get that added
for you to work off of (i.e. Pipe::Writer equality should be enough).
Can you confirm we have tickets for the following:
* Authenticating the /call endpoint.
* Extending the existing framework rate limiting functionality to support http
schedulers.
src/master/master.hpp (line 1236)
https://reviews.apache.org/r/36318/#comment147484
Let's take `Master*` first in both constructors?
src/master/master.hpp (line 1237)
https://reviews.apache.org/r/36318/#comment147483
This doesn't need the `_` anymore?
src/master/master.hpp (line 1248)
https://reviews.apache.org/r/36318/#comment147482
Is this helpful? :)
src/master/master.hpp (line 1259)
https://reviews.apache.org/r/36318/#comment147432
If we omit this, let's add a LOG(FATAL) here. However, not convinced that
we need to omit this, it's pretty straightforward:
```
JSON::Object object = JSON::Protobuf(event);
return stringify(object);
```
At which point, we can stick the returns inside each case block? Or does
the compiler not like that? Really hoping that it figures out that there's
always a return due to the enum class :)
```
switch (contentType) {
case ContentType::PROTOBUF: {
return event.SerializeAsString();
}
case ContentType::JSON: {
JSON::Object object = JSON::Protobuf(event);
return stringify(object);
}
}
```
If not, we can stick an UNREACHABLE at the end.
src/master/master.hpp (lines 1265 - 1266)
https://reviews.apache.org/r/36318/#comment147436
Might as well just do the following, since we have to copy anyway?
```
auto encoder = recordio::Encoderscheduler::Event(serialize);
http = Http();
http.get().writer = writer;
http.get().encoder = encoder;
```
With https://issues.apache.org/jira/browse/MESOS-2757 we can make this look
even cleaner:
```
auto encoder = recordio::Encoderscheduler::Event(serialize);
http = Http();
http-writer = writer;
http-encoder = encoder;
```
src/master/master.hpp (lines 1277 - 1279)
https://reviews.apache.org/r/36318/#comment147443
Don't bother logging this, since it's likely already closed (e.g. framework
being removed). Alternatively, perhaps you only want to close here when
'connected' == true?
If you do want to keep the logging, let's make sure to print the framework
information consistently with our other framework logging.
src/master/master.hpp (line 1326)
https://reviews.apache.org/r/36318/#comment147447
Space here between template and the
src/master/master.hpp (lines 1331 - 1335)
https://reviews.apache.org/r/36318/#comment147448
How about pulling out the Event into an 'event' variable, s/event/encoded/,
and avoiding the `writer_` temporary?
src/master/master.hpp (lines 1336 - 1338)
https://reviews.apache.org/r/36318/#comment147446
Let's print the framework information consistently (use ` *this`). Also
the colon typically comes before the reason, not the ID:
```
LOG(ERROR) Failed to send event to framework *this :
streaming connection closed;
```
Also, do you want to be only sending when 'connected' is true? Are there
cases where we expecting this to get called when 'connected' is false?
src/master/master.hpp (lines 1553 - 1560)
https://reviews.apache.org/r/36318/#comment147450
Can we place the 'pid' and 'http' together and document that exactly one of
these is some?
Also, just to be sure, you're planning to support live upgrades from PID -
HTTP and vice versa?
src/master/master.hpp (lines 1574 - 1578)
https://reviews.apache.org/r/36318/#comment147453
This is a bit unwiedly, how about an if or just doing a ternary on the pid
part of the output?
e.g.
```
stream framework.id() ( framework.info.name() )
if (framework.pid.isSome()) {
stream at framework.pid.get();
}
return stream;
```
Should be easier to read?
src/master/master.cpp (line 956)
https://reviews.apache.org/r/36318/#comment147490
You don't need isSome here, (OptionT, T) equality is
Re: Review Request 36318: Refactored framework struct in master to support http frameworks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 28, 2015, 3:47 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Changes --- Address review comments. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change refactors the framework struct in master to introduce support for http frameworks. - pid becomes a optional field now in the framework struct. - added optional fields for supporting http frameworks to the framework struct Diffs (updated) - src/common/http.hpp 765860fa7d0ce354320e9d293d09719be87efca0 src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070 src/master/master.hpp 2c924addfb4c52d3048ee6ded13ce638145cc93f src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510 Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + tests now go in a separate patch now. Thanks, Anand Mazumdar
Re: Review Request 36318: Refactored framework struct in master to support http frameworks
On July 27, 2015, 11:54 p.m., Ben Mahler wrote:
Nice work Anand!
I left feedback here, but it is all addressed in the diff I sent you over
email. With the diff applied this patch looks like a shippable step to me.
Note that per the comments, I also fenced off addFramework for http
schedulers (much like you've already done here for failoverFramework). In
both of these, we'll need to set up a readerClosed callback (the equivalent
of link()). I also noticed that we'll need connection equality for this, so
I'll get that added for you to work off of (i.e. Pipe::Writer equality
should be enough).
Can you confirm we have tickets for the following:
* Authenticating the /call endpoint.
* Extending the existing framework rate limiting functionality to support
http schedulers.
The Authentication JIRA already exists ( MESOS-2297 ). Would add the rate
limiting JIRA.
On July 27, 2015, 11:54 p.m., Ben Mahler wrote:
src/master/master.cpp, line 1653
https://reviews.apache.org/r/36318/diff/9/?file=1021868#file1021868line1653
Can this really be a CHECK?
E.g.
HTTP framework F is subscribed.
A random 'pid'-based KILL is sent with framework id F.
It seems that in this case, we should drop because it's not from the
subscribed framework, but we'll instead fail this CHECK?
How about:
```
if (framework.pid != from) {
...
}
```
This will handle pid being None.
Good catch, fixed all the other occurences. My bad.
On July 27, 2015, 11:54 p.m., Ben Mahler wrote:
src/master/master.cpp, line 1876
https://reviews.apache.org/r/36318/diff/9/?file=1021868#file1021868line1876
Any reason you added a TODO in the above send, but not here? Seems we
also have the `Framework*` here. I can imagine adding a CHECK to send to
ensure that it's never called with (re-)registration messages when
http.isSome()? Was this the concern?
The TODO was meant for the send(...) above when we did not have a Framework*.
My bad. Moved the comment to the other send(...)
- Anand
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/#review93165
---
On July 28, 2015, 3:47 a.m., Anand Mazumdar wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36318/
---
(Updated July 28, 2015, 3:47 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco
Massenzio, and Vinod Kone.
Bugs: MESOS-2294
https://issues.apache.org/jira/browse/MESOS-2294
Repository: mesos
Description
---
This change refactors the framework struct in master to introduce support for
http frameworks.
- pid becomes a optional field now in the framework struct.
- added optional fields for supporting http frameworks to the framework struct
Diffs
-
src/common/http.hpp 765860fa7d0ce354320e9d293d09719be87efca0
src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070
src/master/master.hpp 2c924addfb4c52d3048ee6ded13ce638145cc93f
src/master/master.cpp a8a195df07b5a97fdba7dfc5f312bbfa85a0d510
Diff: https://reviews.apache.org/r/36318/diff/
Testing
---
make check + tests now go in a separate patch now.
Thanks,
Anand Mazumdar
Re: Review Request 36318: Refactored framework struct in master to support http frameworks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 25, 2015, 7:40 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change refactors the framework struct in master to introduce support for http frameworks. - pid becomes a optional field now in the framework struct. - added optional fields for supporting http frameworks to the framework struct Diffs (updated) - src/common/http.hpp 765860fa7d0ce354320e9d293d09719be87efca0 src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070 src/master/master.hpp 827d0d599912b2936beb9615610f627f6c9a2d43 src/master/master.cpp 613a011e205611702921179b6c436d62447e2dca Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + tests now go in a separate patch now. Thanks, Anand Mazumdar
Re: Review Request 36318: Refactored framework struct in master to support http frameworks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 25, 2015, 6:49 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Changes --- Modified code to use the encoder from MESOS-3067 Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change refactors the framework struct in master to introduce support for http frameworks. - pid becomes a optional field now in the framework struct. - added optional fields for supporting http frameworks to the framework struct Diffs (updated) - src/common/http.hpp 765860fa7d0ce354320e9d293d09719be87efca0 src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070 src/master/master.hpp 827d0d599912b2936beb9615610f627f6c9a2d43 src/master/master.cpp 613a011e205611702921179b6c436d62447e2dca Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + tests now go in a separate patch now. Thanks, Anand Mazumdar
Re: Review Request 36318: Refactored framework struct in master to support http frameworks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 24, 2015, 9:42 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Changes --- Moving subscribeHttpFramework(...)/subscribe(...) in a separate patch. This patch only deals with the framework refactoring now. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description (updated) --- This change refactors the framework struct in master to introduce support for http frameworks. - pid becomes a optional field now in the framework struct. - added optional fields for supporting http frameworks to the framework struct Diffs (updated) - src/common/protobuf_utils.hpp 2e827a0923de83d5cf853a12435b451cc7c55891 src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882 src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + tests now go in a separate patch now. Thanks, Anand Mazumdar
Re: Review Request 36318: Refactored framework struct in master to support http frameworks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 23, 2015, 5:29 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Changes --- Removed extra line-break + added the dependent review for moving struct to end of file (r36733) Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description --- This change refactors the framework struct in master to introduce support for http frameworks. - pid becomes a optional field now in the framework struct. - added optional fields for supporting http frameworks to the framework struct - added a subcribe(...) method that registerFramework(...)/subscribeHttpFramework(...) call into. Similar functionality needs to be added for reregister to call into subscribe too. Diffs (updated) - src/common/protobuf_utils.hpp 2e827a0923de83d5cf853a12435b451cc7c55891 src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882 src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + tests now go in a separate patch now. Thanks, Anand Mazumdar
Re: Review Request 36318: Refactored framework struct in master to support http frameworks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 23, 2015, 4:25 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Changes --- Addressed BenM's comments to refactor the framework struct and made pid options ( see description for a list of changes ), Also splitted the original patch into smaller chunks. Summary (updated) - Refactored framework struct in master to support http frameworks Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description (updated) --- This change refactors the framework struct in master to introduce support for http frameworks. - pid becomes a optional field now in the framework struct. - added optional fields for supporting http frameworks to the framework struct - added a subcribe(...) method that registerFramework(...)/subscribeHttpFramework(...) call into. Similar functionality needs to be added for reregister to call into subscribe too. Apologies for the extended diff ( I moved the framework struct to the end of the file as it used some functionality from the master class. The method was templated and had to implemented in the header file ) Diffs (updated) - src/common/protobuf_utils.hpp 2e827a0923de83d5cf853a12435b451cc7c55891 src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882 src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 Diff: https://reviews.apache.org/r/36318/diff/ Testing (updated) --- make check + tests now go in a separate patch now. Thanks, Anand Mazumdar
Re: Review Request 36318: Refactored framework struct in master to support http frameworks
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36318/ --- (Updated July 23, 2015, 5:47 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Isabel Jimenez, Marco Massenzio, and Vinod Kone. Changes --- rebased diff on top , to reduce the clutter of diff that was seen due to moving the struct to the end of file. Bugs: MESOS-2294 https://issues.apache.org/jira/browse/MESOS-2294 Repository: mesos Description (updated) --- This change refactors the framework struct in master to introduce support for http frameworks. - pid becomes a optional field now in the framework struct. - added optional fields for supporting http frameworks to the framework struct - added a subcribe(...) method that registerFramework(...)/subscribeHttpFramework(...) call into. Similar functionality needs to be added for reregister to call into subscribe too. Diffs (updated) - src/common/protobuf_utils.hpp 2e827a0923de83d5cf853a12435b451cc7c55891 src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882 src/master/http.cpp 6f5ca02c52462495b84e77525a6c88299746ece2 src/master/master.hpp 2343a684402972a8c336c0dcdde0bfaffabe7cec src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0 Diff: https://reviews.apache.org/r/36318/diff/ Testing --- make check + tests now go in a separate patch now. Thanks, Anand Mazumdar
