----------------------------------------------------------- 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::Encoder<scheduler::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::Encoder<scheduler::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, (Option<T>, T) equality is well-defined: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp#L117 src/master/master.cpp (line 1653) <https://reviews.apache.org/r/36318/#comment147455> 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. src/master/master.cpp (line 1800) <https://reviews.apache.org/r/36318/#comment147457> Thanks, I added this for you, should disappear from the diff when you rebase. src/master/master.cpp (line 1833) <https://reviews.apache.org/r/36318/#comment147487> You don't need isSome here, (Option<T>, T) equality is well-defined: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp#L117 src/master/master.cpp (lines 1840 - 1842) <https://reviews.apache.org/r/36318/#comment147460> Hm.. why can't you use Framework::send here? You already have a matching `Framework*` here. Ditto below. src/master/master.cpp (line 1876) <https://reviews.apache.org/r/36318/#comment147464> 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? src/master/master.cpp (line 2026) <https://reviews.apache.org/r/36318/#comment147494> You can swap these to avoid the .get() (since Option<T> != T is defined). src/master/master.cpp (lines 2072 - 2075) <https://reviews.apache.org/r/36318/#comment147495> This fits on one line? src/master/master.cpp (line 2102) <https://reviews.apache.org/r/36318/#comment147496> Hm.. there are other cases where you can use 'framework->send' instead of 'send', any reason why you avoided it? src/master/master.cpp (line 2151) <https://reviews.apache.org/r/36318/#comment147465> Hm.. why can't we have this case drop the message? Seems like this could cause the master to crash in the following situation: http framework F connected stale framework F sends unregister message Seems better to drop than to crash? You can just remove the .get() and this will handle the None case already. src/master/master.cpp (lines 2179 - 2180) <https://reviews.apache.org/r/36318/#comment147466> Ditto here about dropping this instead. src/master/master.cpp (lines 2199 - 2205) <https://reviews.apache.org/r/36318/#comment147499> Why the TODO? Can't we just do this? ``` if (framework->pid.isSome()) { // Remove the framework from authenticated. This is safe because // a framework will always reauthenticate before (re-)registering. authenticated.erase(framework->pid.get()); } else { CHECK_SOME(framework->http); // Close the HTTP connection, which may already have // been closed due to scheduler disconnection. framework->http.get().writer.close(); } ``` src/master/master.cpp (lines 2287 - 2288) <https://reviews.apache.org/r/36318/#comment147500> Ditto just dropping instead. src/master/master.cpp (lines 2342 - 2343) <https://reviews.apache.org/r/36318/#comment147501> Ditto dropping. src/master/master.cpp (line 2348) <https://reviews.apache.org/r/36318/#comment147502> Let's just print `*framework` like the other cases: ``` LOG(WARNING) << "Ignoring launch tasks message for offers " << stringify(offerIds) << " from '" << from << "' because it is not from the" << " registered framework " << *framework; ``` src/master/master.cpp (lines 2897 - 2899) <https://reviews.apache.org/r/36318/#comment147503> Let's help people running across this by being a bit more specific, and can we use getOrElse? ``` // TODO(anand): We set 'pid' to UPID() for http frameworks // as 'pid' was made optional in 0.24.0. In 0.25.0, we // no longer have to set pid here for http frameowrks. message.set_pid(framework->pid.getOrElse(UPID())); ``` src/master/master.cpp (lines 2980 - 2981) <https://reviews.apache.org/r/36318/#comment147504> Ditto here and everywhere else for dropping instead of check failure. src/master/master.cpp (lines 3738 - 3765) <https://reviews.apache.org/r/36318/#comment147507> Why leave this broken..? This is pretty easy to fix: ``` // Send the latest framework pids to the slave. hashset<FrameworkID> ids; foreach (const Task& task, tasks) { Framework* framework = getFramework(task.framework_id()); if (framework != NULL && !ids.contains(framework->id())) { UpdateFrameworkMessage message; message.mutable_framework_id()->MergeFrom(framework->id()); // TODO(anand): We set 'pid' to UPID() for http frameworks // as 'pid' was made optional in 0.24.0. In 0.25.0, we // no longer have to set pid here for http frameowrks. message.set_pid(framework->pid.getOrElse(UPID())); send(slave->pid, message); ids.insert(framework->id()); } } ``` src/master/master.cpp (lines 4763 - 4766) <https://reviews.apache.org/r/36318/#comment147512> Any reason not to add the same CHECK as failoverFramework, to fence this off for now? src/master/master.cpp (line 4815) <https://reviews.apache.org/r/36318/#comment147513> How about we make this self-documenting: ``` CHECK_SOME(framework->pid) << "http framework failover not implemented"; ``` :) src/master/master.cpp (line 4960) <https://reviews.apache.org/r/36318/#comment147514> In the same vein, let's add a TODO for http here: ``` // TODO(anand): For http frameworks, close the connection. ``` Note that we can actually do this one, whereas unlink doesn't exist :) - Ben Mahler On July 25, 2015, 7:40 a.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > 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 > ----- > > 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 > >
