Re: Review Request 55970: Updated the master's HTTP operations to handle MULTI_ROLE changes.

[Benjamin Mahler]

> On Jan. 31, 2017, 6:32 p.m., Michael Park wrote:
> > src/master/validation.cpp, lines 1674-1700
> > 
> >
> > It's not clear to me why we need to unallocate the resources.
> > 
> > Presumably the resources in `Offer::Operation`, and the resources in 
> > `usedResources` should have the same `AllocationInfo` set? Is this not the 
> > case?

There are two cases:

(1) Allocated operation (framework accept)
(2) Unallocated operation (http endpoint)

Case 1 would be handled without changes but case 2 is not handled, so my 
approach is to unallocate both to perform the contains check, since we're only 
interested in the case where the volume is already in use. I'll add a comment 
to clarify this.


- Benjamin


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


On Jan. 26, 2017, 1:08 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55970/
> ---
> 
> (Updated Jan. 26, 2017, 1:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of MULTI_ROLE framework support, allocated
> resources in the master have `Resource.AllocationInfo` set. This
> means that the master's http endpoints that apply operations or
> perform checks between unallocated and allocated resources must
> be updated to continue to function correctly.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 
>   src/master/quota_handler.cpp 6e6e7375219d34e6e8d011a025b5f5d70b87383b 
>   src/master/validation.cpp 5f134b781901f2de6a90fa6a10d42cc7629c27a0 
> 
> Diff: https://reviews.apache.org/r/55970/diff/
> 
> 
> Testing
> ---
> 
> The existing tests pass at the end of this review chain.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55970: Updated the master's HTTP operations to handle MULTI_ROLE changes.

[Michael Park]

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


Fix it, then Ship it!





src/master/validation.cpp (lines 1674 - 1700)


It's not clear to me why we need to unallocate the resources.

Presumably the resources in `Offer::Operation`, and the resources in 
`usedResources` should have the same `AllocationInfo` set? Is this not the case?


- Michael Park


On Jan. 25, 2017, 5:08 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55970/
> ---
> 
> (Updated Jan. 25, 2017, 5:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of MULTI_ROLE framework support, allocated
> resources in the master have `Resource.AllocationInfo` set. This
> means that the master's http endpoints that apply operations or
> perform checks between unallocated and allocated resources must
> be updated to continue to function correctly.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 
>   src/master/quota_handler.cpp 6e6e7375219d34e6e8d011a025b5f5d70b87383b 
>   src/master/validation.cpp 5f134b781901f2de6a90fa6a10d42cc7629c27a0 
> 
> Diff: https://reviews.apache.org/r/55970/diff/
> 
> 
> Testing
> ---
> 
> The existing tests pass at the end of this review chain.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55970: Updated the master's HTTP operations to handle MULTI_ROLE changes.

[Guangya Liu]

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




src/master/http.cpp (lines 4614 - 4615)


I think this can be replaced by the util function in below comments?



src/master/quota_handler.cpp (lines 196 - 200)


Per the comments in /r/55870/ , how about move this to resources_utils.cpp?



src/master/validation.cpp (lines 1654 - 1667)


Did not quite catch up the `TODO` here, can you please elaborate for why 
want such a function?



src/master/validation.cpp (line 1718)


How about 

```
if (unallocated(resources).contains(volume)) {
```

and then kill #1715 ?


- Guangya Liu


On 一月 26, 2017, 1:08 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55970/
> ---
> 
> (Updated 一月 26, 2017, 1:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of MULTI_ROLE framework support, allocated
> resources in the master have `Resource.AllocationInfo` set. This
> means that the master's http endpoints that apply operations or
> perform checks between unallocated and allocated resources must
> be updated to continue to function correctly.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 
>   src/master/quota_handler.cpp 6e6e7375219d34e6e8d011a025b5f5d70b87383b 
>   src/master/validation.cpp 5f134b781901f2de6a90fa6a10d42cc7629c27a0 
> 
> Diff: https://reviews.apache.org/r/55970/diff/
> 
> 
> Testing
> ---
> 
> The existing tests pass at the end of this review chain.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55970: Updated the master's HTTP operations to handle MULTI_ROLE changes.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [55970, 55969, 55968, 55967, 55870, 55910, 55909, 55908, 
55868, 55824, 55866, 55863, 55828, 55829, 55827, 55826, 55825, 54836, 54842]

Failed command: python support/apply-reviews.py -n -r 55870

Error:
2017-01-26 04:26:10 URL:https://reviews.apache.org/r/55870/diff/raw/ 
[47956/47956] -> "55870.patch" [1]
error: patch failed: src/master/master.cpp:6528
error: src/master/master.cpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16857/console

- Mesos Reviewbot


On Jan. 26, 2017, 1:08 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55970/
> ---
> 
> (Updated Jan. 26, 2017, 1:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of MULTI_ROLE framework support, allocated
> resources in the master have `Resource.AllocationInfo` set. This
> means that the master's http endpoints that apply operations or
> perform checks between unallocated and allocated resources must
> be updated to continue to function correctly.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 
>   src/master/quota_handler.cpp 6e6e7375219d34e6e8d011a025b5f5d70b87383b 
>   src/master/validation.cpp 5f134b781901f2de6a90fa6a10d42cc7629c27a0 
> 
> Diff: https://reviews.apache.org/r/55970/diff/
> 
> 
> Testing
> ---
> 
> The existing tests pass at the end of this review chain.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 55970: Updated the master's HTTP operations to handle MULTI_ROLE changes.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
Park.


Repository: mesos


Description
---

With the addition of MULTI_ROLE framework support, allocated
resources in the master have `Resource.AllocationInfo` set. This
means that the master's http endpoints that apply operations or
perform checks between unallocated and allocated resources must
be updated to continue to function correctly.


Diffs
-

  src/master/http.cpp a44621f39cb059e654a56f57f75b38947f3a4230 
  src/master/quota_handler.cpp 6e6e7375219d34e6e8d011a025b5f5d70b87383b 
  src/master/validation.cpp 5f134b781901f2de6a90fa6a10d42cc7629c27a0 

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


Testing
---

The existing tests pass at the end of this review chain.


Thanks,

Benjamin Mahler