Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-05-01 Thread Michael Park
On April 8, 2015, 10:38 p.m., Jie Yu wrote: src/master/validation.cpp, lines 581-584 https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line581 Is this necessary, or will be captured by the resources contains check later? Michael Park wrote: It probably

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-05-01 Thread Michael Park
On April 29, 2015, 10:15 p.m., Jie Yu wrote: src/master/validation.cpp, line 575 https://reviews.apache.org/r/32150/diff/8/?file=944524#file944524line575 This fits into the above line? Fixed. On April 29, 2015, 10:15 p.m., Jie Yu wrote: src/master/validation.cpp, line 598

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-05-01 Thread Michael Park
On April 29, 2015, 10:15 p.m., Jie Yu wrote: src/tests/master_validation_tests.cpp, line 286 https://reviews.apache.org/r/32150/diff/8/?file=944525#file944525line286 One extra blank line here please. Added. - Michael ---

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-05-01 Thread Michael Park
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32150/ --- (Updated May 2, 2015, 12:27 a.m.) Review request for mesos, Alexander

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-30 Thread Alexander Rukletsov
On April 29, 2015, 9:54 a.m., Alexander Rukletsov wrote: src/master/master.cpp, lines 2486-2488 https://reviews.apache.org/r/32150/diff/5-8/?file=920953#file920953line2486 This looks a bit weird. Is it what `clang-format` proposes? Michael Park wrote: It is indeed. Do you

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-29 Thread Alexander Rukletsov
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32150/#review81945 --- Ship it! src/master/master.cpp

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-29 Thread Alexander Rukletsov
On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote: src/master/master.cpp, lines 2458-2459 https://reviews.apache.org/r/32150/diff/5/?file=920953#file920953line2458 These two fields are optional, `principal` doesn't have a default. Do we need to check it? Can a framework

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-29 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32150/#review82045 --- Ship it! src/master/master.cpp

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-29 Thread Michael Park
On April 29, 2015, 10:15 p.m., Jie Yu wrote: src/master/master.cpp, lines 2487-2492 https://reviews.apache.org/r/32150/diff/8/?file=944522#file944522line2487 Why use a tmp variable here? Could you combine with validation below: ``` OptionError error =

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-28 Thread Michael Park
On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote: src/master/master.cpp, lines 2458-2459 https://reviews.apache.org/r/32150/diff/5/?file=920953#file920953line2458 These two fields are optional, `principal` doesn't have a default. Do we need to check it? Can a framework

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-28 Thread Michael Park
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32150/ --- (Updated April 28, 2015, 10:21 p.m.) Review request for mesos, Alexander

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-28 Thread Michael Park
On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote: src/tests/master_validation_tests.cpp, line 328 https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line328 Don't you get an unused variable warning? Hm, looks like I do. Not sure what happpened there. Sorry

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-28 Thread Michael Park
On April 8, 2015, 10:38 p.m., Jie Yu wrote: src/master/validation.cpp, lines 581-584 https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line581 Is this necessary, or will be captured by the resources contains check later? It probably would be checked by `contains`,

Re: Review Request 32150: Enabled the master to handle reservation operations.

2015-04-28 Thread Michael Park
On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote: src/master/validation.cpp, line 555 https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line555 Let's leave a comment here that `resource::validate` not only checks for integrity of the `resources` instance,