Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114241 --- Ship it! Master (952ef6d) is green with this patch.

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114280 --- Ship it! Please also add a NEWS entry so we are sure to highlight

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner
> On Jan. 13, 2016, 3:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79 > > > > > > I'd expect this value populated from the command line arg supplied to > >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
> On Jan. 13, 2016, 11:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79 > > > > > > I'd expect this value populated from the command line arg supplied to >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
> On Jan. 14, 2016, 1:06 a.m., Dmitriy Shirchenko wrote: > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java, > > lines 216-217 > > > > > > Is this intentionally commented out? > >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/ --- (Updated Jan. 13, 2016, 10:02 p.m.) Review request for Aurora, Maxim

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/ --- (Updated Jan. 13, 2016, 10:32 p.m.) Review request for Aurora, Maxim

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114323 --- Ship it! Master (952ef6d) is green with this patch.

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner
> On Jan. 13, 2016, 3:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79 > > > > > > I'd expect this value populated from the command line arg supplied to > >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko
> On Jan. 13, 2016, 11:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, lines 182-184 > > > > > > Does not Mesos expect resource role set when accepting an offer?

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114326 --- src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner
> On Jan. 13, 2016, 3:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79 > > > > > > I'd expect this value populated from the command line arg supplied to > >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114287 --- src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko
> On Jan. 13, 2016, 11:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79 > > > > > > I'd expect this value populated from the command line arg supplied to >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114303 --- This patch does not apply cleanly against master (952ef6d), do you

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko
> On Jan. 13, 2016, 11:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 79 > > > > > > I'd expect this value populated from the command line arg supplied to >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner
> On Jan. 13, 2016, 3:48 p.m., Zhitao Li wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 85 > > > > > > I noticed that this predicate is actually reversed (sorry for the bug). > > >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/ --- (Updated Jan. 13, 2016, 11:50 p.m.) Review request for Aurora, Maxim

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114346 --- Ship it! Master (952ef6d) is green with this patch.

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner
> On Jan. 13, 2016, 4:57 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java, > > lines 93-94 > > > > > > This does not read right to me. If it's

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114379 --- Ship it! Master (952ef6d) is green with this patch.

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Dmitriy Shirchenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114353 ---

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/#review114348 --- Only minor comments left.

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
> On Jan. 14, 2016, 12:57 a.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java, > > lines 93-94 > > > > > > This does not read right to me. If it's

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
> On Jan. 14, 2016, 1:06 a.m., Dmitriy Shirchenko wrote: > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java, > > lines 216-217 > > > > > > Is this intentionally commented out? Yes

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
> On Jan. 14, 2016, 12:57 a.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java, > > lines 93-94 > > > > > > This does not read right to me. If it's

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Bill Farner
> On Jan. 13, 2016, 5:06 p.m., Dmitriy Shirchenko wrote: > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java, > > lines 216-217 > > > > > > Is this intentionally commented out? > >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
> On Jan. 13, 2016, 2:57 a.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 129 > > > > > > This is a good general-purpose implementation of this behavior, but in > >

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

2016-01-13 Thread Zhitao Li
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42126/ --- (Updated Jan. 13, 2016, 5:50 p.m.) Review request for Aurora, Maxim