Re: Review Request 32509: Documented the scheduler Event/Call protobufs.

2015-04-22 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32509/#review81266
---


Patch looks great!

Reviews applied: [32500, 32501, 32502, 32504, 32843, 32505, 32506, 32844, 
32845, 33465, 32509]

All tests passed.

- Mesos ReviewBot


On April 23, 2015, 2:55 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32509/
> ---
> 
> (Updated April 23, 2015, 2:55 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-1127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> 783a63ad1cc0edd86605d638046fb959cb6e97e8 
> 
> Diff: https://reviews.apache.org/r/32509/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 32509: Documented the scheduler Event/Call protobufs.

2015-04-22 Thread Vinod Kone 


> On April 22, 2015, 11:26 p.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, line 52
> > 
> >
> > Hm, what do you mean by "can be offered"? Do you want to just s/that 
> > can be// ?

done.


> On April 22, 2015, 11:26 p.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, lines 54-55
> > 
> >
> > Hm, this last sentence sort of implies no optimistic offers? We'll need 
> > to be sure that we update the documentation later I suppose.

yup.


> On April 22, 2015, 11:26 p.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, lines 60-63
> > 
> >
> > "no longer valid" seems a little specific, we could rescind in cases 
> > where the offer would have remained valid but we choose to change the 
> > resource allocations dynamically. How about just s/is no longer valid/is 
> > rescinded/ and removing the "hence" part?
> > 
> > Also, do we need the example or can we trim this down a bit?

"A Rescind is received when an offer is rescinded" doesn't seem to offer much 
value to framework writers? I used "invalid" because from framework's 
perspective the offer is no longer valid, as in, it cannot use it. Does that 
make sense?


> On April 22, 2015, 11:26 p.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, line 69
> > 
> >
> > "or mesos"? Slave and master might be a bit too implementation specific 
> > in the future, plus we'd like to rename "Slave".
> > 
> > On that note, before we finalize the http api, can we get rid of 
> > "slave" in the api?

"mesos" might be weird because frameworks might not consider executors sending 
updates as getting from mesos? Regarding Slave renaming, we should have a 
separate discussion around it because the api uses protobufs from mesos.proto 
which uses it.


> On April 22, 2015, 11:26 p.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, lines 101-102
> > 
> >
> > Specifically, we have to think about how to do reconciliation between 
> > mesos and the scheduler, yes?

yup. expanded the TODO.


> On April 22, 2015, 11:26 p.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, line 151
> > 
> >
> > Do you want to add something about how the "removes" the framework too?

done.


> On April 22, 2015, 11:26 p.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, lines 235-238
> > 
> >
> > Hm.. are we expecting schedulers to use the source to decide whether to 
> > acknowledge? That seems a bit implicit, should we make 'uuid' optional to 
> > make it more clear?
> > 
> > Also, that way, the master can tell when one of its updates is being 
> > acknowledged (empty uuid), which should also make it possible for the 
> > master to drop the acknowledgments, yes?

SGTM. I'll remove uuid from Update, now that it is part of TaskStatus. Will do 
that in a dependent review.


> On April 22, 2015, 11:26 p.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, lines 268-270
> > 
> >
> > Hm.. schedulers can't use Update as a replacement for this Message, 
> > since Message is scheduler -> executor but Update is executor -> scheduler..

Alex asked the same thing earlier. My response was that schedulers could use 
tasks and Updates to do 2 way communication. I'll just remove this sentence to 
avoid confusion.


> On April 22, 2015, 11:26 p.m., Ben Mahler wrote:
> > include/mesos/scheduler/scheduler.proto, lines 113-116
> > 
> >
> > Isn't this more general than these two specific cases? Maybe we should 
> > describe it more generally instead of documenting examples like this? For 
> > example, what should the scheduler do when Error is received? Will the 
> > connection be closed? Will they need to re-subscribe?

This is there for backwards compatibility with the old driver and hence talks 
about the conditions that generate Error with the old driver. I plan to remove 
this in the new API (as the TODO says) because the errors will be delivered as 
HTTP responses instead of on the SUBSCRIBE stream. That makes sense?


- Vinod


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32509/#review81234
---


On April 22, 2015, 9:44 p.m

Re: Review Request 32509: Documented the scheduler Event/Call protobufs.

2015-04-22 Thread Vinod Kone 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32509/
---

(Updated April 23, 2015, 2:55 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

benm's. NNFR.


Bugs: MESOS-1127
https://issues.apache.org/jira/browse/MESOS-1127


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/scheduler/scheduler.proto 
783a63ad1cc0edd86605d638046fb959cb6e97e8 

Diff: https://reviews.apache.org/r/32509/diff/


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 32509: Documented the scheduler Event/Call protobufs.

2015-04-22 Thread Ben Mahler 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32509/#review81234
---

Ship it!


Looks great, only 1 question which we can tackle separately from this review:

Is it possible to describe when to acknowledge an update in a more explicit way 
(currently source == SLAVE of source == EXECUTOR seems implicit). Can we make 
the 'uuid' optional? Also we should think about how the master can drop 
acknowledgements for its own updates.


include/mesos/scheduler/scheduler.proto


Hm, what do you mean by "can be offered"? Do you want to just s/that can 
be// ?



include/mesos/scheduler/scheduler.proto


Hm, this last sentence sort of implies no optimistic offers? We'll need to 
be sure that we update the documentation later I suppose.



include/mesos/scheduler/scheduler.proto


"no longer valid" seems a little specific, we could rescind in cases where 
the offer would have remained valid but we choose to change the resource 
allocations dynamically. How about just s/is no longer valid/is rescinded/ and 
removing the "hence" part?

Also, do we need the example or can we trim this down a bit?



include/mesos/scheduler/scheduler.proto


"or mesos"? Slave and master might be a bit too implementation specific in 
the future, plus we'd like to rename "Slave".

On that note, before we finalize the http api, can we get rid of "slave" in 
the api?



include/mesos/scheduler/scheduler.proto


Specifically, we have to think about how to do reconciliation between mesos 
and the scheduler, yes?



include/mesos/scheduler/scheduler.proto


Isn't this more general than these two specific cases? Maybe we should 
describe it more generally instead of documenting examples like this? For 
example, what should the scheduler do when Error is received? Will the 
connection be closed? Will they need to re-subscribe?



include/mesos/scheduler/scheduler.proto


Do you want to add something about how the "removes" the framework too?



include/mesos/scheduler/scheduler.proto


Hm.. are we expecting schedulers to use the source to decide whether to 
acknowledge? That seems a bit implicit, should we make 'uuid' optional to make 
it more clear?

Also, that way, the master can tell when one of its updates is being 
acknowledged (empty uuid), which should also make it possible for the master to 
drop the acknowledgments, yes?



include/mesos/scheduler/scheduler.proto


Hm.. schedulers can't use Update as a replacement for this Message, since 
Message is scheduler -> executor but Update is executor -> scheduler..


- Ben Mahler


On April 22, 2015, 9:44 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32509/
> ---
> 
> (Updated April 22, 2015, 9:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-1127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> 783a63ad1cc0edd86605d638046fb959cb6e97e8 
> 
> Diff: https://reviews.apache.org/r/32509/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 32509: Documented the scheduler Event/Call protobufs.

2015-04-22 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32509/#review81225
---


Patch looks great!

Reviews applied: [32500, 32501, 32502, 32504, 32843, 32505, 32506, 32844, 
32845, 32509]

All tests passed.

- Mesos ReviewBot


On April 22, 2015, 9:44 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32509/
> ---
> 
> (Updated April 22, 2015, 9:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-1127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> 783a63ad1cc0edd86605d638046fb959cb6e97e8 
> 
> Diff: https://reviews.apache.org/r/32509/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 32509: Documented the scheduler Event/Call protobufs.

2015-04-22 Thread Vinod Kone 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32509/
---

(Updated April 22, 2015, 9:44 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

updated documentation.


Bugs: MESOS-1127
https://issues.apache.org/jira/browse/MESOS-1127


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/scheduler/scheduler.proto 
783a63ad1cc0edd86605d638046fb959cb6e97e8 

Diff: https://reviews.apache.org/r/32509/diff/


Testing
---

make check


Thanks,

Vinod Kone