Re: Review Request 39848: Validate revocable resources

[Niklas Nielsen]

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



src/slave/slave.cpp (lines 1485 - 1524)


+1 to Vinod's comment on 'validation.cpp'.

Another suggestion which doesn't require a large refactor, could be to move 
some of the comparison code into the Resources class itself. That way, it could 
be something like: if(_resources_revocable_total < 
task.resources().revocable()) { ... }

Also, any reason this need to happen in this continuation of runTask() and 
not the earlier one?


- Niklas Nielsen


On Jan. 19, 2016, 2:39 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39848/
> ---
> 
> (Updated Jan. 19, 2016, 2:39 a.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-2647
> https://issues.apache.org/jira/browse/MESOS-2647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validates if there are enough revocable resources when launching
> the task on slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 
> 
> Diff: https://reviews.apache.org/r/39848/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39848: Validate revocable resources

[Guangya Liu]

> On 一月 19, 2016, 1:31 p.m., Niklas Nielsen wrote:
> > src/slave/slave.cpp, lines 1485-1524
> > 
> >
> > +1 to Vinod's comment on 'validation.cpp'.
> > 
> > Another suggestion which doesn't require a large refactor, could be to 
> > move some of the comparison code into the Resources class itself. That way, 
> > it could be something like: if(_resources_revocable_total < 
> > task.resources().revocable()) { ... }
> > 
> > Also, any reason this need to happen in this continuation of runTask() 
> > and not the earlier one?

Thanks Niklas, I will upload a new patch later and will use `contain` to check 
if the reources are available for the task.


- Guangya


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


On 一月 19, 2016, 10:39 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39848/
> ---
> 
> (Updated 一月 19, 2016, 10:39 a.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-2647
> https://issues.apache.org/jira/browse/MESOS-2647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validates if there are enough revocable resources when launching
> the task on slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 
> 
> Diff: https://reviews.apache.org/r/39848/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39848: Validate revocable resources

[Guangya Liu]

> On 十二月 5, 2015, 12:21 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1503
> > 
> >
> > I think at this point it's worthwhile to factor out the validations in 
> > this function into slave/validation.cpp file like we have done in 
> > master/validation.cpp. The validation should likely return an object 
> > (TaskStatus?) that contains the message and REASON to be used in the status 
> > update.
> > 
> > This would also help with unit testing the validations more easily.

OK, will factor out the validations to slave/validation.cpp. Can you please 
help check if the logic is OK here? As I was using one double value to minus 
another, shall we use CHECK_NEAR as the solution in 
https://reviews.apache.org/r/40767/diff/1#index_header ? Thanks!


- Guangya


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


On 十一月 5, 2015, 7:36 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39848/
> ---
> 
> (Updated 十一月 5, 2015, 7:36 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2647
> https://issues.apache.org/jira/browse/MESOS-2647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validates if there are enough revocable resources when launching
> the task on slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 
> 
> Diff: https://reviews.apache.org/r/39848/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39848: Validate revocable resources

[Guangya Liu]

> On 十二月 5, 2015, 12:21 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1503
> > 
> >
> > I think at this point it's worthwhile to factor out the validations in 
> > this function into slave/validation.cpp file like we have done in 
> > master/validation.cpp. The validation should likely return an object 
> > (TaskStatus?) that contains the message and REASON to be used in the status 
> > update.
> > 
> > This would also help with unit testing the validations more easily.
> 
> Guangya Liu wrote:
> OK, will factor out the validations to slave/validation.cpp. Can you 
> please help check if the logic is OK here? As I was using one double value to 
> minus another, shall we use CHECK_NEAR as the solution in 
> https://reviews.apache.org/r/40767/diff/1#index_header ? Thanks!

Vinod, after a second think, putting the validation here may be better as the 
current validation does not fit into validate() logic in master/validation.cpp 
slave/validation.cpp well. All of the validate() APIs are returning Error, it 
may make the logic not clear if we put the above logic to slave/validation.cpp. 
Actually the current _runTask also have some validateion logic, such as 
checkpointedResources, checkpointedExecutorResources etc. What do you say? 
Thanks.


- Guangya


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


On 十一月 5, 2015, 7:36 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39848/
> ---
> 
> (Updated 十一月 5, 2015, 7:36 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2647
> https://issues.apache.org/jira/browse/MESOS-2647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validates if there are enough revocable resources when launching
> the task on slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 
> 
> Diff: https://reviews.apache.org/r/39848/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39848: Validate revocable resources

[Vinod Kone]

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



src/slave/slave.cpp (line 1503)


I think at this point it's worthwhile to factor out the validations in this 
function into slave/validation.cpp file like we have done in 
master/validation.cpp. The validation should likely return an object 
(TaskStatus?) that contains the message and REASON to be used in the status 
update.

This would also help with unit testing the validations more easily.


- Vinod Kone


On Nov. 5, 2015, 7:36 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39848/
> ---
> 
> (Updated Nov. 5, 2015, 7:36 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2647
> https://issues.apache.org/jira/browse/MESOS-2647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validates if there are enough revocable resources when launching
> the task on slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 
> 
> Diff: https://reviews.apache.org/r/39848/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39848: Validate revocable resources

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39845, 39848]

All tests passed.

- Mesos ReviewBot


On Nov. 5, 2015, 7:36 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39848/
> ---
> 
> (Updated Nov. 5, 2015, 7:36 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2647
> https://issues.apache.org/jira/browse/MESOS-2647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validates if there are enough revocable resources when launching
> the task on slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 
> 
> Diff: https://reviews.apache.org/r/39848/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39848: Validate revocable resources

[Guangya Liu]

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

(Updated 十一月 5, 2015, 7:36 a.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Validates if there are enough revocable resources when launching
the task on slave.


Diffs (updated)
-

  src/slave/slave.cpp ddeece4334b13cf8b5ab7d87012bbf946640025b 

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


Testing
---

make
make check


Thanks,

Guangya Liu