Re: Review Request 69563: Made master process authorization results properly for offer operations.

2018-12-16 Thread Chun-Hung Hsiao 


> On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp
> > Lines 4757-4758 (original), 4757-4758 (patched)
> > 
> >
> > Performing this change in this patch makes it hard to review the 
> > important change. It should be in a separate patch.
> > 
> > I personally am not sure we want to make this change as the reference 
> > semantics it introduces make it harder to verify that we e.g., do not 
> > access invalid references. This might not be worth the performance 
> > improvement this patch might bring.
> 
> Chun-Hung Hsiao wrote:
> That's why I have a `CHECK(authorizationIter != authorizations->end())` 
> everywhere before dereferencing. Since `authorizations` is const-qualified, 
> we won't be able to remove or modify any future, so in this aspect it's safer 
> compared to copying and poping futures. Note that in the original code we 
> never check if the deque is nonempty.

Moved the change to r/69571.


> On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp
> > Lines 5513-5519 (patched)
> > 
> >
> > This patch now first authorizes and then validates. Is this the 
> > accepted pattern? In any case, this seems to be a change which would 
> > warrent its own patch.
> 
> Chun-Hung Hsiao wrote:
> This is the pattern we do for all other operations. And yes I'll create a 
> separated ticket for `LAUNCH_GROUP` and split this into another patch.

Moved the change to r/69571.


> On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp
> > Lines 5805 (patched)
> > 
> >
> > We should log some descriptive message here.

Resolved in r/69571.


- Chun-Hung


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


On Dec. 14, 2018, 1:55 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69563/
> ---
> 
> (Updated Dec. 14, 2018, 1:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, James DeFelice, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-9474
> https://issues.apache.org/jira/browse/MESOS-9474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where the master does not complete the
> processing of authorization results for `CREATE_DISK`, `DESTROY_DISK`
> and `LAUNCH_GROUP`, causing other operations to drop if a remaining
> authorization is denied..
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c 
> 
> 
> Diff: https://reviews.apache.org/r/69563/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69563: Made master process authorization results properly for offer operations.

2018-12-14 Thread Chun-Hung Hsiao 


> On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp
> > Lines 4757-4758 (original), 4757-4758 (patched)
> > 
> >
> > Performing this change in this patch makes it hard to review the 
> > important change. It should be in a separate patch.
> > 
> > I personally am not sure we want to make this change as the reference 
> > semantics it introduces make it harder to verify that we e.g., do not 
> > access invalid references. This might not be worth the performance 
> > improvement this patch might bring.

That's why I have a `CHECK(authorizationIter != authorizations->end())` 
everywhere before dereferencing. Since `authorizations` is const-qualified, we 
won't be able to remove or modify any future, so in this aspect it's safer 
compared to copying and poping futures. Note that in the original code we never 
check if the deque is nonempty.


> On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp
> > Lines 5513-5519 (patched)
> > 
> >
> > This patch now first authorizes and then validates. Is this the 
> > accepted pattern? In any case, this seems to be a change which would 
> > warrent its own patch.

This is the pattern we do for all other operations. And yes I'll create a 
separated ticket for `LAUNCH_GROUP` and split this into another patch.


> On Dec. 14, 2018, 8:58 a.m., Benjamin Bannier wrote:
> > src/master/master.cpp
> > Lines 5667 (patched)
> > 
> >
> > This change and the one below seem in `DESTROY_DISK` seem to be the 
> > central pieces to address MESOS-9474.

I'll split this into two patches. See above.


- Chun-Hung


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


On Dec. 14, 2018, 1:55 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69563/
> ---
> 
> (Updated Dec. 14, 2018, 1:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, James DeFelice, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-9474
> https://issues.apache.org/jira/browse/MESOS-9474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where the master does not complete the
> processing of authorization results for `CREATE_DISK`, `DESTROY_DISK`
> and `LAUNCH_GROUP`, causing other operations to drop if a remaining
> authorization is denied..
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c 
> 
> 
> Diff: https://reviews.apache.org/r/69563/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69563: Made master process authorization results properly for offer operations.

2018-12-14 Thread Benjamin Bannier 

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



Could you please add a test, and break out refactorings and unrelated cleanups 
into their own patch?


src/master/master.cpp
Lines 4757-4758 (original), 4757-4758 (patched)


Performing this change in this patch makes it hard to review the important 
change. It should be in a separate patch.

I personally am not sure we want to make this change as the reference 
semantics it introduces make it harder to verify that we e.g., do not access 
invalid references. This might not be worth the performance improvement this 
patch might bring.



src/master/master.cpp
Lines 5513-5519 (patched)


This patch now first authorizes and then validates. Is this the accepted 
pattern? In any case, this seems to be a change which would warrent its own 
patch.



src/master/master.cpp
Lines 5667 (patched)


This change and the one below seem in `DESTROY_DISK` seem to be the central 
pieces to address MESOS-9474.



src/master/master.cpp
Lines 5805 (patched)


We should log some descriptive message here.


- Benjamin Bannier


On Dec. 14, 2018, 2:55 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69563/
> ---
> 
> (Updated Dec. 14, 2018, 2:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, James DeFelice, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-9474
> https://issues.apache.org/jira/browse/MESOS-9474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where the master does not complete the
> processing of authorization results for `CREATE_DISK`, `DESTROY_DISK`
> and `LAUNCH_GROUP`, causing other operations to drop if a remaining
> authorization is denied..
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c 
> 
> 
> Diff: https://reviews.apache.org/r/69563/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69563: Made master process authorization results properly for offer operations.

2018-12-13 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 69563 was successfully built and tested.

Reviews applied: `['69563']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2706/mesos-review-69563

- Mesos Reviewbot Windows


On Dec. 14, 2018, 1:55 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69563/
> ---
> 
> (Updated Dec. 14, 2018, 1:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, James DeFelice, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-9474
> https://issues.apache.org/jira/browse/MESOS-9474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where the master does not complete the
> processing of authorization results for `CREATE_DISK`, `DESTROY_DISK`
> and `LAUNCH_GROUP`, causing other operations to drop if a remaining
> authorization is denied..
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 3de0fd35cc815f4b5787ee2cb5e81f5059d7a47c 
> 
> 
> Diff: https://reviews.apache.org/r/69563/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>