> 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
> 
>

Reply via email to