mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 32150: Enabled the master to handle reservation operations.
Date Tue, 28 Apr 2015 19:30:51 GMT


> 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 about that.


> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_validation_tests.cpp, line 176
> > <https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line176>
> >
> >     const?

These variables have been removed as per Jie request.


> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_validation_tests.cpp, line 249
> > <https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line249>
> >
> >     const? here and below.

These variables have been removed as per Jie's request.


> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/tests/master_validation_tests.cpp, line 314
> > <https://reviews.apache.org/r/32150/diff/5/?file=920956#file920956line314>
> >
> >     You don't use `principal` in unreserve, is this comment valid then? As I have
already mentioned, I would rather check for principal match though.

As you point out, the 2 tests were actually functionally equivalent so I've collapsed them
into one. See my comment above as for why we don't check the `principal` match.


> On April 22, 2015, 12:38 p.m., Alexander Rukletsov wrote:
> > src/master/validation.cpp, lines 591-592
> > <https://reviews.apache.org/r/32150/diff/5/?file=920955#file920955line591>
> >
> >     Shall we check framework and request principals are the same?

No, at least not in the current specification. The `principal` indicates the `principal` of
the operator or framework that reserved the resources, which means that  we need to make sure
that `Resource.Reservation.principal` and `FrameworkInfo.principal` matches for the `Reserve`
operation. On `Unreserve`, we use the operator or framework's `principal` and the `principal`
set on the `Resource` object in conjunction with the "unreserve" ACL to determine whether
the unreserve operation is authorized. In the absense of an ACL, we allow anyone to unreserve
any resources. This is a matter of authorization however, so we wouldn't check for the `prinicpal`
matching in `validate` even with ACLs.


- Michael


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


On April 27, 2015, 10:10 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> -----------------------------------------------------------
> 
> (Updated April 27, 2015, 10:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2139
>     https://issues.apache.org/jira/browse/MESOS-2139
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Handled reservation operations in `Master::_accept`.
> 
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp d42a6f321c88ec5d0418264bdda39d083ff54a7e 
>   src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec 
>   src/master/validation.cpp dc25995bf57397d42fcde458414f0402d19bf792 
>   src/tests/master_validation_tests.cpp 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395 
> 
> Diff: https://reviews.apache.org/r/32150/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message