Re: Review Request 68548: Added allocator benchmark test harness.

2018-09-04 Thread Meng Zhu 

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



For the description, let's be more informative, how about:

Introduced a base class for writing allocator benchmarks.
The test harness encapsulates the common logic in the allocator
benchmarks such has adding frameworks and agents. One only
needs to customize the various options in the `Benchmarkconfig`
to set up the test cluster.


src/tests/hierarchical_allocator_benchmarks.cpp
Lines 141 (patched)


Since we are touching the allocator option and random sorter is probably 
interesting for the benchmark you added in https://reviews.apache.org/r/68591, 
can you customize this using the helper here:


https://github.com/apache/mesos/blob/master/src/master/allocator/allocator.cpp#L35

I think you might need to introduce a new option in the `Bencmarkconfig`.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 143 (patched)


Let's add a comment here regarding what this is for, and show an example of 
framework/agent id.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 182 (patched)


Ah, I did not expect this, my bad.

Initialize will unpause the allocator:

https://github.com/apache/mesos/blob/6705b31187d2441cb62670bd0be4904896c8b695/src/master/allocator/mesos/hierarchical.cpp#L170

So we need to pause after initializing.

Per my comments above regarding `create()` the allocator in the base class, 
it no longer makes sense to ask the user to pause. So let's:

(1) create the allocator here according to the config
(2) initialize it
(3) pause it
(4) add frameworks and agents.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 184 (patched)


`CHECK_NOTNONE` instead of unguarded get.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 202 (patched)


hmm, I thought we removed the `replace` call along with the restriction 
that the user needs to name their profiles `...-#`. I bet this will lead to 
some grumpy debugging sessions:)

Can we just let users name their profiles at will and just concatenate the 
delineator?

e.g. I can just name my framework profile "framework" and you are free to 
process it to "framework-3".

I think most users would not care about the naming, as long as the naming 
is not surprising and documented, it should be fine.

ditto for the agent.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 255-256 (patched)


One thing that is missing for the existing benchmarks is agent's used 
resources, e.g.:


https://github.com/apache/mesos/blob/6f5ff9550d975ed0adc2a12f7690a52be3a47afa/src/tests/hierarchical_allocator_tests.cpp#L6253-L6268

Can you add that option as well? Let's try to make the prototype usable for 
the existing benchmarks.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 278-279 (patched)


I feel there are some ambiguities as to which parameters are kept/copied in 
the benchmark base class, which are kept just in the `Benchmarkconfig`, e.g. it 
is not clear to me why we keep the "profiles" here. They are not needed after 
the initialization phase.

I wonder should we just keep a copy of the benchmarkconfig here?

Any better idea?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 281-282 (patched)


Add `const` to both.


- Meng Zhu


On Aug. 28, 2018, 12:45 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> ---
> 
> (Updated Aug. 28, 2018, 12:45 p.m.)
> 
> 
> Review request for mesos, Meng Zhu and Till Toenshoff.
> 
> 
> Bugs: MESOS-9187
> https://issues.apache.org/jira/browse/MESOS-9187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new harness allows us to run allocators with different framework and
> agent profiles.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
>   src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68548/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 68548: Added allocator benchmark test harness.

2018-08-31 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 68548 was successfully built and tested.

Reviews applied: `['68549', '68548']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2269/mesos-review-68548

- Mesos Reviewbot Windows


On Aug. 28, 2018, 7:45 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> ---
> 
> (Updated Aug. 28, 2018, 7:45 p.m.)
> 
> 
> Review request for mesos, Meng Zhu and Till Toenshoff.
> 
> 
> Bugs: MESOS-9187
> https://issues.apache.org/jira/browse/MESOS-9187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new harness allows us to run allocators with different framework and
> agent profiles.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
>   src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68548/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 68548: Added allocator benchmark test harness.

2018-08-28 Thread Meng Zhu 

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



Glad to see this being added! Thank you!

I did a pass on the test harness. Will look into the test after the harness is 
updated.


src/tests/hierarchical_allocator_benchmarks.cpp
Lines 58 (patched)


Do we need this? Can you audit other "using/include" and remove any that 
are not needed?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 82 (patched)


`set` would enable us to benchmark multi-role frameworks.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 85 (patched)


Take reference?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 91 (patched)


We can use `CHECK_NOTERROR` instead of the unguarded `get`.

Moreover, I think it is more flexible to just take a `Resource` object 
here, leaving the parsing or whatever to the caller. For non-trivial resource 
objects e.g. involving persistent volume, they are more likely to already be 
resource objects instead of a plain string.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 109 (patched)


ditto resources object.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 135-136 (patched)


Comparing to the original test harness, we probably want to control more 
things e.g. random allocator and other flag values in the future.

I suggest selectively keep/expose several fields such as interval, sorter 
type and etc. explicitly instead of keeping a default master flag. See my 
comments regarding `initializeCluster` below.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 155 (patched)


Are we relying on callers to name their frameworks ...%pid? Then this is 
very fragile. One solution is to just concatenate with `_#` and document it 
well. I think "nextFrameworkId" is used for that?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 160 (patched)


ditto



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 162 (patched)


If we `push_back` last, we can std::move.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 167 (patched)


I would expect `frameworkTasksLaunched` tracks 
the number of "launched tasks", not "tasks to launch".

Can you also add some comments to the fields whose meaning is not 
immediately obvious?



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 183 (patched)


ditto



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 190 (patched)


We can std::move here, even in the test, savings are savings:)



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 197 (patched)


I think this TODO is no longer relavent.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 199-200 (patched)


As mentioned above, let's expose more knobs here. For now, maybe just 
allocator type, allocation interval, offercallback and any other that are 
currently being touched.

This will likely make the initialize function signature ugly. It is 
probably better to introduce a flag (maybe utilize the `BenchmarkConfig` 
below?). You probably can come up with a better solution.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 220 (patched)


ah, one thing I forget to mention in the original patch. This vector here 
is fragile. It will be concurrently accessed by both allocator actor and the 
test actor. Like the test base, let's use an asynchronous queue here.


https://github.com/apache/mesos/blob/master/src/tests/hierarchical_allocator_tests.cpp#L282



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 241 (patched)


Create frameworks and agents. (Or just remove the comment).



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 345-346 (patched)


The meaning of these fields are not obvious, adding some comments would be 
helpful.



src/tests/hierarchical_allocator_benchmarks.cpp
Lines 350 (patched)


hmm, just to understand the test

Re: Review Request 68548: Added allocator benchmark test harness.

2018-08-28 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 68548 was successfully built and tested.

Reviews applied: `['68549', '68548']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2253/mesos-review-68548

- Mesos Reviewbot Windows


On Aug. 28, 2018, 12:45 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68548/
> ---
> 
> (Updated Aug. 28, 2018, 12:45 p.m.)
> 
> 
> Review request for mesos, Meng Zhu and Till Toenshoff.
> 
> 
> Bugs: MESOS-9187
> https://issues.apache.org/jira/browse/MESOS-9187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new harness allows us to run allocators with different framework and
> agent profiles.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
>   src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68548/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Review Request 68548: Added allocator benchmark test harness.

2018-08-28 Thread Kapil Arya 

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

Review request for mesos, Meng Zhu and Till Toenshoff.


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


Repository: mesos


Description
---

The new harness allows us to run allocators with different framework and
agent profiles.


Diffs
-

  src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
  src/tests/hierarchical_allocator_benchmarks.cpp PRE-CREATION 


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


Testing
---


Thanks,

Kapil Arya