-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30612/#review70982
-----------------------------------------------------------
Thanks for the patch, Alexander! I have a high-level question. In such checks:
```
Try<JSON::Value> expected = JSON::parse("[]");
ASSERT_SOME(expected);
Result<JSON::Value> _tasks = framework.find<JSON::Value>("tasks");
EXPECT_SOME_EQ(expected.get(), _tasks);
```
do we want instead (or additionally) to check that the task array is empty? If
possible, I would prefer to operate with high-level abstractions here and leave
JSON representation details for JSON tests.
src/master/master.cpp
<https://reviews.apache.org/r/30612/#comment116461>
Shouldn't it be `/frameworks.json`?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116479>
Isn't it more stable to check that `frameworks` key is present and the
associated array is empty?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116462>
What "black magic" do you mean?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116463>
Where is the second slave mentioned above? Is it started implicitly somehow?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116464>
Though this is what you see in other tests, there is a nicer way to do it:
`EXPECT_FALSE(offers.get().empty());`
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116465>
You can cache the offer references before and reuse it here and below.
`Offer offer = offers.get()[0];`
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116475>
Rephrase, please. E.g. "Specific order is not required".
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116483>
Please use `DEFAULT_EXECUTOR_ID` instead of hard constant.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116476>
Let's add a message here and below via `<<` to add some verbosity.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116473>
Shouldn't we check the array is empty?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116477>
See comment above.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116481>
s/frameworks registered/registered frameworks/ ?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116482>
Ditto, misleading comment?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116484>
Isn't it covered in a previous test?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116485>
See above.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116486>
See my comment to the previous test.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/30612/#comment116491>
Ditto.
- Alexander Rukletsov
On Feb. 4, 2015, 1:34 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30612/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 1:34 p.m.)
>
>
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Till Toenshoff.
>
>
> Bugs: MESOS-2157
> https://issues.apache.org/jira/browse/MESOS-2157
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds endpoints for paths /master/frameworks/{framework}/tasks/{task}.
>
> Works with [29883](https://reviews.apache.org/r/29883).
>
> Builds on the abandoned patch 14286.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3981b18
> src/master/master.hpp cd37ee9
> src/master/master.cpp d04b2c4
> src/tests/master_tests.cpp 678d27f
>
> Diff: https://reviews.apache.org/r/30612/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
