Re: Review Request 49517: Implement GetState V1 master API.

[Anand Mazumdar]

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


Fix it, then Ship it!




Would fix these minor style nits while committing.


src/master/http.cpp (line 1552)


2 space indent here.


- Anand Mazumdar


On July 6, 2016, 3:56 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49517/
> ---
> 
> (Updated July 6, 2016, 3:56 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The help function will also be used for snapshot of
> event stream.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp debedd4a4061034a3b55181e93443b9d5e676c52 
>   src/master/master.hpp 996ff425fc8e9234a5e02560460ad1233dca7061 
>   src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 
> 
> Diff: https://reviews.apache.org/r/49517/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49517: Implement GetState V1 master API.

[haosdent huang]

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




src/master/http.cpp (lines 1551 - 1552)


I think @anandmazumdar means
```
.then([contentType](const mesos::master::Response::GetState& getState)
-> Future {
```


- haosdent huang


On July 6, 2016, 1:48 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49517/
> ---
> 
> (Updated July 6, 2016, 1:48 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The help function will also be used for snapshot of
> event stream.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp debedd4a4061034a3b55181e93443b9d5e676c52 
>   src/master/master.hpp 996ff425fc8e9234a5e02560460ad1233dca7061 
>   src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 
> 
> Diff: https://reviews.apache.org/r/49517/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49517: Implement GetState V1 master API.

[Zhitao Li]

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

(Updated July 6, 2016, 1:48 a.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Rebase and review comments.


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


Repository: mesos


Description
---

The help function will also be used for snapshot of
event stream.


Diffs (updated)
-

  src/master/http.cpp debedd4a4061034a3b55181e93443b9d5e676c52 
  src/master/master.hpp 996ff425fc8e9234a5e02560460ad1233dca7061 
  src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 

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


Testing
---

make check


Thanks,

Zhitao Li

Re: Review Request 49517: Implement GetState V1 master API.

[Anand Mazumdar]

> On July 6, 2016, 12:30 a.m., Anand Mazumdar wrote:
> > src/tests/api_tests.cpp, line 537
> > 
> >
> > Just execDriver->sendXX would suffice.
> > 
> > Would be a good idea to do a sweep across this file and remove the 
> > redundant `get()` in favor of the `->` operator.
> 
> Zhitao Li wrote:
> I think we need at least one `get()` here, because type of `execDriver` 
> is a Future?

Yep, my bad. Might be good to audit the other instances where they are not 
needed.


- Anand


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


On July 4, 2016, 4:05 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49517/
> ---
> 
> (Updated July 4, 2016, 4:05 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The help function will also be used for snapshot of
> event stream.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
>   src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
>   src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 
> 
> Diff: https://reviews.apache.org/r/49517/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49517: Implement GetState V1 master API.

[Zhitao Li]

> On July 6, 2016, 12:30 a.m., Anand Mazumdar wrote:
> > src/tests/api_tests.cpp, line 537
> > 
> >
> > Just execDriver->sendXX would suffice.
> > 
> > Would be a good idea to do a sweep across this file and remove the 
> > redundant `get()` in favor of the `->` operator.

I think we need at least one `get()` here, because type of `execDriver` is a 
Future?


- Zhitao


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


On July 4, 2016, 4:05 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49517/
> ---
> 
> (Updated July 4, 2016, 4:05 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The help function will also be used for snapshot of
> event stream.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
>   src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
>   src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 
> 
> Diff: https://reviews.apache.org/r/49517/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49517: Implement GetState V1 master API.

[Anand Mazumdar]

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


Fix it, then Ship it!




Some minor style nits.

Also if you have not done so, can you run this test with `gtest_repeat=100` to 
be sure that it's not flaky?


src/master/http.cpp (line 1537)


2 space indent here for continuations please like you do in a later 
function in this change.



src/master/http.cpp (line 1538)


2 space indent here before `->`



src/master/http.cpp (line 1550)


pass argument by const ref. Ditto for the declaration in header



src/master/http.cpp (line 1558)


Do you need to capture anything here?



src/tests/api_tests.cpp (line 453)


Remove this. We tend to avoid being explicit about `Times(1)`.



src/tests/api_tests.cpp (line 467)


Extra space.



src/tests/api_tests.cpp (line 474)


New line before and capture by const ref.



src/tests/api_tests.cpp (lines 477 - 478)


Reorder expectation based on comment earlier.



src/tests/api_tests.cpp (lines 481 - 489)


Let's be less verbose and just use:

`createTask(offer, "", DEFAULT_EXECUTOR_ID)`



src/tests/api_tests.cpp (line 517)


Newline before and capture by const ref?



src/tests/api_tests.cpp (line 537)


Just execDriver->sendXX would suffice.

Would be a good idea to do a sweep across this file and remove the 
redundant `get()` in favor of the `->` operator.



src/tests/api_tests.cpp (line 552)


Newline before + capture by const ref?


- Anand Mazumdar


On July 4, 2016, 4:05 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49517/
> ---
> 
> (Updated July 4, 2016, 4:05 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The help function will also be used for snapshot of
> event stream.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
>   src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
>   src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 
> 
> Diff: https://reviews.apache.org/r/49517/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49517: Implement GetState V1 master API.

[Zhitao Li]

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

(Updated July 4, 2016, 4:05 a.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Adjust EXPECT_CALL location.


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


Repository: mesos


Description
---

The help function will also be used for snapshot of
event stream.


Diffs (updated)
-

  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
  src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 

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


Testing
---

make check


Thanks,

Zhitao Li

Re: Review Request 49517: Implement GetState V1 master API.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/api_tests.cpp (lines 560 - 561)


should be set before the driver.stop() ?


- Vinod Kone


On July 3, 2016, 7:34 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49517/
> ---
> 
> (Updated July 3, 2016, 7:34 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The help function will also be used for snapshot of
> event stream.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
>   src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
>   src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 
> 
> Diff: https://reviews.apache.org/r/49517/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49517: Implement GetState V1 master API.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On July 3, 2016, 7:34 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49517/
> ---
> 
> (Updated July 3, 2016, 7:34 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The help function will also be used for snapshot of
> event stream.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
>   src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
>   src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 
> 
> Diff: https://reviews.apache.org/r/49517/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 49517: Implement GetState V1 master API.

[Zhitao Li]

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

(Updated July 3, 2016, 7:34 a.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Fix indent.


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


Repository: mesos


Description
---

The help function will also be used for snapshot of
event stream.


Diffs (updated)
-

  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
  src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 

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


Testing
---

make check


Thanks,

Zhitao Li

Re: Review Request 49517: Implement GetState V1 master API.

[Zhitao Li]

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

(Updated July 2, 2016, 12:19 a.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

The help function will also be used for snapshot of
event stream.


Diffs (updated)
-

  src/master/http.cpp 7b2f77b9264242f31ab62eb9db7c621a1b8aa2fe 
  src/master/master.hpp be7cd239c49a0710a29a8187c01484f9f6d615e1 
  src/tests/api_tests.cpp e2d8bf591667ec9d8c609e55a424b55561892b5f 

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


Testing
---

make check


Thanks,

Zhitao Li

Re: Review Request 49517: Implement GetState V1 master API.

[Vinod Kone]

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




src/master/http.cpp (line 1428)


fix this in the previous review.



src/master/http.cpp (lines 1536 - 1537)


ditto. see earlier review for comments.



src/master/http.cpp (lines 1565 - 1569)


this should be moved to GetLeadingMaster



src/master/http.cpp (line 1571)


s/instead/instead of/



src/tests/api_tests.cpp (lines 430 - 433)


mvoe this to #465



src/tests/api_tests.cpp (lines 533 - 534)


move this to #549.



src/tests/api_tests.cpp (line 537)


copy paste error? update the comment.



src/tests/api_tests.cpp (line 546)


a task goes to completed after the ACK for terminal state is processed by 
the master. you need to setup an expectation on StatusUpdateAcknowledgement 
message to guarantee that. See `GetTasks` test for an example.


- Vinod Kone


On July 1, 2016, 4:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49517/
> ---
> 
> (Updated July 1, 2016, 4:09 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The help function will also be used for snapshot of
> event stream.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 528f01f2e00ddbd15da1cc4fca27b5347c9fc798 
>   src/master/master.hpp 7388a3e7f3a0c9f0a2a286a8c8107210971b654a 
>   src/tests/api_tests.cpp a7f075172f870bcd3ac7fb9c7a69b70ac5cb73c5 
> 
> Diff: https://reviews.apache.org/r/49517/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>