----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36078/#review90080 -----------------------------------------------------------
Looks good overall, mainly curious if 'force' should be optional, given it doesn't make sense for HA frameworks. include/mesos/scheduler/scheduler.proto (line 167) <https://reviews.apache.org/r/36078/#comment143087> Hm.. first in which context? For HTTP, the scheduler has to keep a working Subscribe connection to receive streaming events. Outside of the context of HTTP, if we're doing the old-style message passing, then they only need to send this whenever they detect a change or a disconnection with the master, yes? Maybe it's worth it to start adding HTTP specific comments in here, since that's what we're likely going to move to? include/mesos/scheduler/scheduler.proto (lines 169 - 173) <https://reviews.apache.org/r/36078/#comment143090> As it's worded now, it seems a bit hard to get the higher level picture, maybe rephrase given my other suggestion below? ``` // New schedulers will be assigned an identifier by the master during // the initial Subscribe call, by leaving 'framework_info.id' empty. // In order to re-subscribe once an identifier has been assigned during // the initial subscription, the scheduler must set 'framework_info.id' // to be correctly identified across disconnections, failovers, etc. ``` Thoughts? include/mesos/scheduler/scheduler.proto (lines 174 - 177) <https://reviews.apache.org/r/36078/#comment143088> Should this note be located on FrameworkInfo instead? include/mesos/scheduler/scheduler.proto (lines 180 - 188) <https://reviews.apache.org/r/36078/#comment143092> Hm.. per my other comments below, do we want to say that this is only relevent for HA frameworks? Should this be optional with a default? include/mesos/scheduler/scheduler.proto (lines 287 - 290) <https://reviews.apache.org/r/36078/#comment143082> Not yours, but as this is worded now, it seems a little hard to understand that the master assigns an identifier for the scheduler to use during its lifetime (across failovers, etc). Here something to work off of: ``` // New schedulers will be assigned an identifier by the master during // the initial Subscribe call. Once assigned, the scheduler must set // the 'framework_id' here and within its FrameworkInfo in any further // Subscribe calls. This allow the master to identify it correctly // across disconnections, failovers, etc. ``` The thinking here is that it might be helpful for people to understand what this identifier is for, and how / when it is assigned. Knowing this seems to make a little bit easier to intuit when they need to be setting this? include/mesos/scheduler/scheduler.proto (line 290) <https://reviews.apache.org/r/36078/#comment143079> I think we can remove the '()' in these snippets, since they are language specific anyway (e.g. python doesn't have them in the generated proto code). src/examples/event_call_framework.cpp (line 309) <https://reviews.apache.org/r/36078/#comment143084> If you were to write a comment as to why 'force' is set to true here, what would you say? It seems a little odd for non-HA frameworks to set 'force', since the force semantics are not applicable to them. Should this be optional, or how should we deal with this? Ditto for the other cases where force is being set to true. src/scheduler/scheduler.cpp (line 196) <https://reviews.apache.org/r/36078/#comment143085> Looks like the following would be consistent with the rest of the messages below: ``` drop(call, "Expecting 'framework_id' to be present"); ``` This will print the following: ``` Dropping ACCEPT: Expecting 'framework_id' to be present ``` vs. ``` Dropping ACCEPT: Call is missing FrameworkID ``` src/scheduler/scheduler.cpp (line 228) <https://reviews.apache.org/r/36078/#comment143086> newline here but not in the else? - Ben Mahler On July 1, 2015, 5:23 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36078/ > ----------------------------------------------------------- > > (Updated July 1, 2015, 5:23 a.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, and Ben Mahler. > > > Bugs: MESOs-2551 > https://issues.apache.org/jira/browse/MESOs-2551 > > > Repository: mesos > > > Description > ------- > > Subscribe message includes 'FrameworkInfo' and 'force'. Top level protobuf > has FrameworkID. > > > Diffs > ----- > > include/mesos/scheduler/scheduler.proto > 249ec532b53fe428b7e66be4ced8223e66535b49 > src/examples/event_call_framework.cpp > 63e42bc83ccc0e4085d7619c478e5b010a49098a > src/master/master.cpp 34ce744f84465ecc9aeecd5fdc3d06047a4b7d92 > src/sched/sched.cpp 7563abb85819b0b2bc9afdfd810b33c923c2522e > src/scheduler/scheduler.cpp f360e4d062488986b14e3d48d140996e8ed9e7d6 > src/tests/scheduler_tests.cpp cbe6c91a1b4f864ceb11cf062da0ada6c9666f9f > > Diff: https://reviews.apache.org/r/36078/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
