Re: Review Request 43824: Addressed comments of 41672.

[Adam B]

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




src/tests/hierarchical_allocator_tests.cpp (lines 236 - 239)


Comparing `int` vs. `unsigned` lead to the ReviewBot error you got. I'd 
just stick with `int`, since it's shorter and the compiler will probably 
optimize all this out anyway.


- Adam B


On March 8, 2016, 7:17 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 8, 2016, 7:17 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

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

(Updated March 9, 2016, 3:17 a.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Addressed the comments of Adam.


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


Repository: mesos


Description
---

Addressed comments of 41672.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

make && make check successfully.


Thanks,

Yongqiao Wang

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

> On March 8, 2016, 11:20 p.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 233
> > 
> >
> > `getAllocations` is too general a name for what this function actually 
> > does. More like `handleTwoAllocationsAndRecoverResources`. Any method that 
> > starts with `get` should return something, preferably whatever comes after 
> > `get` (i.e. `Allocations`).
> > If you parameterize the number of iterations of the loop, add a 
> > boolean/enum for whether to recover the resources, and return 
> > `frameworkAllocations`, then you could call it `getAllocations`, and use it 
> > for the 3-iteration loop at the end of the test.
> > That said, only two reuses may not be worth pulling it out (although 3 
> > may be).
> > I give you 3 options (your choice):
> > 1. Rename the method appropriately (may require code changes so it's 
> > not as ugly as `handleTwoAllocationsAndRecoverResources`).
> > 2. Inline the method back into the test in both places.
> > 3. Generalize the method and reuse it for the 3rd instance.

Thanks very much for your detailed description.


- Yongqiao


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


On March 8, 2016, 12:10 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 8, 2016, 12:10 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Adam B]

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




src/tests/hierarchical_allocator_tests.cpp (line 233)


`getAllocations` is too general a name for what this function actually 
does. More like `handleTwoAllocationsAndRecoverResources`. Any method that 
starts with `get` should return something, preferably whatever comes after 
`get` (i.e. `Allocations`).
If you parameterize the number of iterations of the loop, add a 
boolean/enum for whether to recover the resources, and return 
`frameworkAllocations`, then you could call it `getAllocations`, and use it for 
the 3-iteration loop at the end of the test.
That said, only two reuses may not be worth pulling it out (although 3 may 
be).
I give you 3 options (your choice):
1. Rename the method appropriately (may require code changes so it's not as 
ugly as `handleTwoAllocationsAndRecoverResources`).
2. Inline the method back into the test in both places.
3. Generalize the method and reuse it for the 3rd instance.


- Adam B


On March 8, 2016, 4:10 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 8, 2016, 4:10 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43824]

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

- Mesos ReviewBot


On March 8, 2016, 12:10 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 8, 2016, 12:10 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

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

(Updated March 8, 2016, 12:10 p.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Addressed comments of Adam.


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


Repository: mesos


Description
---

Addressed comments of 41672.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

make && make check successfully.


Thanks,

Yongqiao Wang

Re: Review Request 43824: Addressed comments of 41672.

[Adam B]

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


Fix it, then Ship it!




Just a bunch a grammar fixes. Otherwise shippable.


src/tests/hierarchical_allocator_tests.cpp (line 2445)


s/all/each/
s/half of resources/half of the resources/



src/tests/hierarchical_allocator_tests.cpp (line 2446)


s/role's weight/roles' weights/



src/tests/hierarchical_allocator_tests.cpp (line 2458)


s/due to/since/



src/tests/hierarchical_allocator_tests.cpp (line 2490)


s/,/;/



src/tests/hierarchical_allocator_tests.cpp (lines 2495 - 2496)


s|the 1/3 resources|1/3 of the resources|
s|the 2/3 resources|2/3 of the resources|



src/tests/hierarchical_allocator_tests.cpp (line 2496)


s/role's weight/roles' weights/



src/tests/hierarchical_allocator_tests.cpp (line 2516)


Seems like we repeat the contents of this loop a few times. Could all/part 
of the contents be worth factoring out into a reusable helper function?



src/tests/hierarchical_allocator_tests.cpp (line 2545)


s/,/;/



src/tests/hierarchical_allocator_tests.cpp (lines 2550 - 2551)


s|the X/6 resources|X/6 of the resources|g



src/tests/hierarchical_allocator_tests.cpp (line 2552)


s/role's weight/roles' weights/g



src/tests/hierarchical_allocator_tests.cpp (line 2559)


s/due to/because/



src/tests/hierarchical_allocator_tests.cpp (line 2605)


s/,/;/


- Adam B


On March 7, 2016, 5:55 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 7, 2016, 5:55 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Adam B]

> On March 3, 2016, 1:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2607-2609
> > 
> >
> > I'm an ESL, but having both "per weight" and "by weight" sounds a bit 
> > strange. Maybe Adam can help wiht finding the right preposition.
> 
> Yongqiao Wang wrote:
> Thanks Alex. @Adam, I am also an ESL. What is your suggestion for this?

s/per role's weight/according to each role's weight/
s/number of resources by their role's weight/appropriate number of resources/


> On March 3, 2016, 1:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2652-2654
> > 
> >
> > How about
> > 
> > // Framework2 registers with 'role2' which also uses the default 
> > weight. It
> > // will not get any offers because all resources are offered to 
> > `framework1`.
> 
> Yongqiao Wang wrote:
> I remember I wrote this comment like you before. The current comment is 
> changed with Adam's comments. @Adam, any further comments for this?

I'm fine with either, but Alex's is shorter.


- Adam


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


On March 7, 2016, 5:55 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 7, 2016, 5:55 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43824]

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

- Mesos ReviewBot


On March 7, 2016, 1:55 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 7, 2016, 1:55 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

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

(Updated March 7, 2016, 1:55 p.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Addressed comments of Alex.


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


Repository: mesos


Description
---

Addressed comments of 41672.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

make && make check successfully.


Thanks,

Yongqiao Wang

Re: Review Request 43824: Addressed comments of 41672.

[Alexander Rukletsov]

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




src/tests/hierarchical_allocator_tests.cpp (line 2445)


Backtick variable names, please. Here and in other newly added comments.


- Alexander Rukletsov


On March 7, 2016, 11:31 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 7, 2016, 11:31 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43824]

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

- Mesos ReviewBot


On March 7, 2016, 11:31 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 7, 2016, 11:31 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

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

(Updated March 7, 2016, 11:31 a.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Addressed comments of Alex.


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


Repository: mesos


Description
---

Addressed comments of 41672.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

make && make check successfully.


Thanks,

Yongqiao Wang

Re: Review Request 43824: Addressed comments of 41672.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (lines 2444 - 2446)


Nice! We use and treat `{}` blocks as subtests, hence they need a leading 
comment explaining what is going to happen (what is being tested), similar to 
tests themselves.



src/tests/hierarchical_allocator_tests.cpp (line 2456)


Please move "ensure" onto the previous line.



src/tests/hierarchical_allocator_tests.cpp (lines 2492 - 2494)


See my comment above.



src/tests/hierarchical_allocator_tests.cpp (lines 2545 - 2547)


Ditto.


- Alexander Rukletsov


On March 3, 2016, 2:05 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 3, 2016, 2:05 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

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

(Updated March 3, 2016, 2:05 p.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Addressed comments of Alex.


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


Repository: mesos


Description
---

Addressed comments of 41672.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

make && make check successfully.


Thanks,

Yongqiao Wang

Re: Review Request 43824: Addressed comments of 41672.

[Alexander Rukletsov]

> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2635-2640
> > 
> >
> > Does it fit one line? I know we are a bit inconsistent about it, but 
> > let's prefer oneliners where it doesn't impact the readability.
> 
> Yongqiao Wang wrote:
> Put those parameters into one line will exceed 80 chars, then `git 
> commit` will failed due to the default check. I agree with your suggestion, 
> but I think we should have a discussion in community to enhance the related 
> check rules.

How come `allocator->addSlave(agent.id(), agent, None(), agent.resources(), 
{});` does not fit in 80 chars, even prefixed with 4 spaces?


> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2736-2738
> > 
> >
> > I think these lines are good candidate to go under the `{}` together 
> > with the checks. This way you can avoid numeral suffixes and have a 1:1 
> > relation between `{}`-blocks and test cases.
> > 
> > Does it make sense?
> 
> Yongqiao Wang wrote:
> Update weights should be a completely different logic with the above 
> check, so it would not be proper to put them together.

I'm talking about the checks *below*, those, where you observe the effect of 
setting weights.


- Alexander


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


On March 3, 2016, 1:28 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 3, 2016, 1:28 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

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

(Updated March 3, 2016, 1:28 p.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Addressed comments of Alex.


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


Repository: mesos


Description
---

Addressed comments of 41672.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 

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


Testing
---

make && make check successfully.


Thanks,

Yongqiao Wang

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2607-2609
> > 
> >
> > I'm an ESL, but having both "per weight" and "by weight" sounds a bit 
> > strange. Maybe Adam can help wiht finding the right preposition.

Thanks Alex. @Adam, I am also an ESL. What is your suggestion for this?


> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2635-2640
> > 
> >
> > Does it fit one line? I know we are a bit inconsistent about it, but 
> > let's prefer oneliners where it doesn't impact the readability.

Put those parameters into one line will exceed 80 chars, then `git commit` will 
failed due to the default check. I agree with your suggestion, but I think we 
should have a discussion in community to enhance the related check rules.


> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2652-2654
> > 
> >
> > How about
> > 
> > // Framework2 registers with 'role2' which also uses the default 
> > weight. It
> > // will not get any offers because all resources are offered to 
> > `framework1`.

I remember I wrote this comment like you before. The current comment is changed 
with Adam's comments. @Adam, any further comments for this?


> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 2850
> > 
> >
> > Why do we need to `resume` here?

It does not needed, I have removed this line.


> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2736-2738
> > 
> >
> > I think these lines are good candidate to go under the `{}` together 
> > with the checks. This way you can avoid numeral suffixes and have a 1:1 
> > relation between `{}`-blocks and test cases.
> > 
> > Does it make sense?

Update weights should be a completely different logic with the above check, so 
it would not be proper to put them together.


> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2790-2792
> > 
> >
> > See my comment above about how to avoid numeral suffixes.

Same comments as above.


- Yongqiao


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


On March 1, 2016, 6:42 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 1, 2016, 6:42 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Alexander Rukletsov]

> On March 3, 2016, 9:03 a.m., Alexander Rukletsov wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2759-2770
> > 
> >
> > Let me clarify what I meant in r/41672/. I would like to avoid the 
> > `if-else` dispatch based on the framework id. It becomes even worse below, 
> > where we have three frameworks. Here is what I suggest:
> > ```
> > {
> > hashmap currentAllocations;
> > Resources totalAllocatedResources;
> > 
> > for (unsigned i = 0; i < 2; i++) {
> >   Future allocation = allocations.get();
> >   AWAIT_READY(allocation);
> > 
> >   totalAllocatedResources += 
> > Resources::sum(allocation.get().resources);
> >   currentAllocations[allocation.get().frameworkId] = 
> > allocation.get();
> > 
> >   // Recover the allocated resources so they can be offered again 
> > next time.
> >   foreachpair (const SlaveID& slaveId,
> >const Resources& resources,
> >allocation.get().resources) {
> > allocator->recoverResources(
> > allocation.get().frameworkId,
> > slaveId,
> > resources,
> > None());
> >   }
> > }
> > 
> > //
> > ASSERT_EQ(2u, currentAllocations[framework1.id()].resources.size());
> > EXPECT_EQ(Resources::parse(DOUBLE_RESOURCES).get(),
> >   
> > Resources::sum(currentAllocations[framework1.id()].resources));
> > 
> > //
> > ASSERT_EQ(4u, currentAllocations[framework2.id()].resources.size());
> > EXPECT_EQ(Resources::parse(FOURFOLD_RESOURCES).get(),
> >   
> > Resources::sum(currentAllocations[framework2.id()].resources));
> > 
> > // Check to ensure that these two allocations sum to the total 
> > resources,
> > // this check can ensure there are only two allocations in this 
> > case.
> > EXPECT_EQ(Resources::parse(TOTAL_RESOURCES).get(), 
> > totalAllocatedResources);
> > EXPECT_EQ(hashset(expectedFrameworkIds),
> >   currentAllocations.keys());
> >   }
> > ```
> > 
> > Note I don't explicitly use `expectedFrameworkIds`.

A better name for `currentAllocations` is probably `frameworkAllocations`.

We can also add extra checks, if you want:
`ASSERT_TRUE(currentAllocations.contains(framework1.id()));`

And there is a typo in the note above, it should read: "Note I don't explicitly 
use `frameworkIds` set."


- Alexander


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


On March 1, 2016, 6:42 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated March 1, 2016, 6:42 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

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

(Updated March 1, 2016, 6:42 a.m.)


Review request for mesos, Adam B and Alexander Rukletsov.


Changes
---

Addressed comments of Adam.


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


Repository: mesos


Description
---

Addressed comments of 41672.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
5f771f02db9bd098f3cd36730cd84bf2f5e87a33 

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


Testing
---

make && make check successfully.


Thanks,

Yongqiao Wang

Re: Review Request 43824: Addressed comments of 41672.

[Yongqiao Wang]

> On Feb. 28, 2016, 9:39 a.m., Adam B wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 2658-2661
> > 
> >
> > Can you say 100% and 0% instead of 1 and 0? And how is framework1's 
> > "share" '6'? Is that the number of agents/offers, rather than the 
> > framework's "share"?

I refer to the styles in above tests in this file? for example: 
https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp?262#L262
 so I used 1 rather than 100%. I think we should keep consistent, especially in 
a same file. For framework's "share", I have updated them to a ratio.


- Yongqiao


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


On Feb. 22, 2016, 7:23 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated Feb. 22, 2016, 7:23 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Adam B]

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



Thanks for following up with this. AlexR is on vacation now, so I'll review 
these changes. I only had a few suggestions.
After reviewing the issues AlexR opened up on the previous patch, the only 
remaining question is whether to use a `std::set` instead of a 
`hashmap counts;`.


src/tests/hierarchical_allocator_tests.cpp (lines 2647 - 2649)


Please fix the wrapping to 70 or 80 chars instead of 60.



src/tests/hierarchical_allocator_tests.cpp (lines 2653 - 2656)


Can you say 100% and 0% instead of 1 and 0? And how is framework1's "share" 
'6'? Is that the number of agents/offers, rather than the framework's "share"?



src/tests/hierarchical_allocator_tests.cpp (lines 2733 - 2736)


It shouldn't be necessary to advance the clock if updateWeights will notice 
the change and 'rebalance' immediately, right?



src/tests/hierarchical_allocator_tests.cpp (lines 2793 - 2797)


Please comment why we don't manually advance the clock here. I guess 
`updateWeights` won't call `allocate()` since no framework exists in 'role3' 
yet, but `addFramework` will.


- Adam B


On Feb. 21, 2016, 11:23 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated Feb. 21, 2016, 11:23 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 43824: Addressed comments of 41672.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43824]

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

- Mesos ReviewBot


On Feb. 22, 2016, 7:23 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> ---
> 
> (Updated Feb. 22, 2016, 7:23 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> ---
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>