Re: Review Request 57340: Remove adjustment code within Resources::apply.

2017-03-16 Thread Benjamin Mahler 

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


Ship it!




Ship It!

- Benjamin Mahler


On March 15, 2017, 3:26 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57340/
> ---
> 
> (Updated March 15, 2017, 3:26 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7048
> https://issues.apache.org/jira/browse/MESOS-7048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove adjustment code within Resources::apply.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
>   src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
>   src/master/allocator/mesos/hierarchical.cpp 
> 37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/resources_tests.cpp 5ffc9e7111c91157b79a38a1cbd8d58a0565e450 
>   src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 
> 
> 
> Diff: https://reviews.apache.org/r/57340/diff/3/
> 
> 
> Testing
> ---
> 
> WIP
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 57340: Remove adjustment code within Resources::apply.

2017-03-14 Thread Jay Guo 

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

(Updated March 15, 2017, 11:26 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

rebase


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


Repository: mesos


Description
---

Remove adjustment code within Resources::apply.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/master/allocator/mesos/hierarchical.cpp 
37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
  src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
  src/tests/resources_tests.cpp 5ffc9e7111c91157b79a38a1cbd8d58a0565e450 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 


Diff: https://reviews.apache.org/r/57340/diff/3/

Changes: https://reviews.apache.org/r/57340/diff/2-3/


Testing
---

WIP


Thanks,

Jay Guo

Re: Review Request 57340: Remove adjustment code within Resources::apply.

2017-03-14 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [57340]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On March 14, 2017, 5:39 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57340/
> ---
> 
> (Updated March 14, 2017, 5:39 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7048
> https://issues.apache.org/jira/browse/MESOS-7048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove adjustment code within Resources::apply.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
>   src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
>   src/master/allocator/mesos/hierarchical.cpp 
> 37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/resources_tests.cpp 5ffc9e7111c91157b79a38a1cbd8d58a0565e450 
>   src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 
> 
> 
> Diff: https://reviews.apache.org/r/57340/diff/2/
> 
> 
> Testing
> ---
> 
> WIP
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 57340: Remove adjustment code within Resources::apply.

2017-03-14 Thread Jay Guo 

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

(Updated March 15, 2017, 1:39 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Address bmahler's comments.


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


Repository: mesos


Description
---

Remove adjustment code within Resources::apply.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/master/allocator/mesos/hierarchical.cpp 
37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
  src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
  src/tests/resources_tests.cpp 5ffc9e7111c91157b79a38a1cbd8d58a0565e450 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 


Diff: https://reviews.apache.org/r/57340/diff/2/

Changes: https://reviews.apache.org/r/57340/diff/1-2/


Testing
---

WIP


Thanks,

Jay Guo

Re: Review Request 57340: Remove adjustment code within Resources::apply.

2017-03-14 Thread Jay Guo 


> On March 14, 2017, 7:03 a.m., Benjamin Mahler wrote:
> > Looks good, the changes to resources.cpp were done how? Are they a direct 
> > reversion to the old code?

Yes, I did a `git revert` and apply changes based on that.


> On March 14, 2017, 7:03 a.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.hpp
> > Lines 126-128 (original), 126-128 (patched)
> > 
> >
> > I like the naming you used for the strip function, mind also clarifying 
> > the naming of this one in a separate patch?
> > 
> > ```
> > void injectAllocationInfo(
> > Offer::Operation* operation,
> > const Resource::AllocationInfo& allocationInfo);
> > ```

Will do.


- Jay


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


On March 15, 2017, 1:39 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57340/
> ---
> 
> (Updated March 15, 2017, 1:39 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7048
> https://issues.apache.org/jira/browse/MESOS-7048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove adjustment code within Resources::apply.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
>   src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
>   src/master/allocator/mesos/hierarchical.cpp 
> 37fb7a62ef95c2f6e5a9e9a4ab49260332e2b03b 
>   src/master/master.cpp d43350d08ddd14fb7ba2a79c899abda6a864038c 
>   src/tests/resources_tests.cpp 5ffc9e7111c91157b79a38a1cbd8d58a0565e450 
>   src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 
> 
> 
> Diff: https://reviews.apache.org/r/57340/diff/2/
> 
> 
> Testing
> ---
> 
> WIP
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 57340: Remove adjustment code within Resources::apply.

2017-03-13 Thread Benjamin Mahler 

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



Looks good, the changes to resources.cpp were done how? Are they a direct 
reversion to the old code?


src/common/protobuf_utils.hpp
Lines 126-128 (original), 126-128 (patched)


I like the naming you used for the strip function, mind also clarifying the 
naming of this one in a separate patch?

```
void injectAllocationInfo(
Offer::Operation* operation,
const Resource::AllocationInfo& allocationInfo);
```



src/common/protobuf_utils.hpp
Lines 131 (patched)


// This strips the `Resource::AllocationInfo` from all
// `Resource` objects contained within the operation.



src/common/protobuf_utils.cpp
Lines 448 (patched)


How about just "strip"?



src/master/allocator/mesos/hierarchical.cpp
Lines 813-818 (original), 813-821 (patched)


Can you update the comment to reflect why we have to do the stripping?



src/master/master.cpp
Line 9230 (original), 9230-9233 (patched)


Mind adding a comment here to help the reader?

```
// We need to strip the allocation info from the operation's
// resources in order to apply the operation successfully
// since the agent's total is stored as unallocated resources.
```


- Benjamin Mahler


On March 6, 2017, 3:26 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57340/
> ---
> 
> (Updated March 6, 2017, 3:26 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7048
> https://issues.apache.org/jira/browse/MESOS-7048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove adjustment code within Resources::apply.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
>   src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
>   src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
>   src/master/allocator/mesos/hierarchical.cpp 
> dcafc79421fec179f274aff05da516e64447c924 
>   src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
>   src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f 
>   src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 
> 
> 
> Diff: https://reviews.apache.org/r/57340/diff/1/
> 
> 
> Testing
> ---
> 
> WIP
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 57340: Remove adjustment code within Resources::apply.

2017-03-06 Thread Jay Guo 

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Remove adjustment code within Resources::apply.


Diffs
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/master/allocator/mesos/hierarchical.cpp 
dcafc79421fec179f274aff05da516e64447c924 
  src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
  src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 


Diff: https://reviews.apache.org/r/57340/diff/1/


Testing
---

WIP


Thanks,

Jay Guo