> On July 1, 2015, 7:52 p.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, line 167 > > <https://reviews.apache.org/r/36078/diff/1/?file=996383#file996383line167> > > > > 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?
rephrased without mentioning HTTP. PTAL. > On July 1, 2015, 7:52 p.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, lines 174-177 > > <https://reviews.apache.org/r/36078/diff/1/?file=996383#file996383line174> > > > > Should this note be located on FrameworkInfo instead? moved. > On July 1, 2015, 7:52 p.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, lines 180-188 > > <https://reviews.apache.org/r/36078/diff/1/?file=996383#file996383line180> > > > > 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? I dont think a default is safe because either always 'true' or always 'false' can result in unexpected behavior. I have added a recommnedation instead to guide framework writers. > On July 1, 2015, 7:52 p.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, lines 287-290 > > <https://reviews.apache.org/r/36078/diff/1/?file=996383#file996383line287> > > > > 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? done. thanks. > On July 1, 2015, 7:52 p.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, line 290 > > <https://reviews.apache.org/r/36078/diff/1/?file=996383#file996383line290> > > > > 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). n/a. > On July 1, 2015, 7:52 p.m., Ben Mahler wrote: > > src/examples/event_call_framework.cpp, line 309 > > <https://reviews.apache.org/r/36078/diff/1/?file=996384#file996384line309> > > > > 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. see my above comment regarding 'force' being optional. > On July 1, 2015, 7:52 p.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 196 > > <https://reviews.apache.org/r/36078/diff/1/?file=996387#file996387line196> > > > > 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 > > ``` done > On July 1, 2015, 7:52 p.m., Ben Mahler wrote: > > src/scheduler/scheduler.cpp, line 235 > > <https://reviews.apache.org/r/36078/diff/1/?file=996387#file996387line235> > > > > newline here but not in the else? oops. fixed > On July 1, 2015, 7:52 p.m., Ben Mahler wrote: > > include/mesos/scheduler/scheduler.proto, lines 169-173 > > <https://reviews.apache.org/r/36078/diff/1/?file=996383#file996383line169> > > > > 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? pointed to the comments on fraemwork id to avoid duplication. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36078/#review90080 ----------------------------------------------------------- On July 1, 2015, 9:43 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36078/ > ----------------------------------------------------------- > > (Updated July 1, 2015, 9:43 p.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/mesos.proto 5ab3c4a305562af16af804e7ab77e576b96e468d > 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 > >
