Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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

(Updated June 18, 2016, 2:33 a.m.)


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


Changes
---

rebase


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


Repository: mesos


Description
---

Implement v1::master::Call::GET_TASKS.


Diffs (updated)
-

  include/mesos/v1/master/master.proto 5a6bbeb0f9b57d03803e366df32e04e9598e00ed 
  src/internal/evolve.hpp 875ce89f80056c03fc9a248248114dbb471b517a 
  src/internal/evolve.cpp dc2d2c882b1794f33c8649a42fb454caf848ab36 
  src/master/http.cpp dca814b070e513420175a2df6db7844fd425c3fa 
  src/master/master.hpp 2bddfe1c6638281e3ed34146490bc33f66d87d5c 
  src/tests/api_tests.cpp 41fef2fcc4bd2a53e2de25c89d6f1250852cdc23 

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


Testing
---

make check on ubuntu 14.04


Thanks,

Jay Guo

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On June 17, 2016, 6:59 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 17, 2016, 6:59 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master/master.proto 
> 5a6bbeb0f9b57d03803e366df32e04e9598e00ed 
>   src/internal/evolve.hpp 5f3e267fa3a0420fcbed7be6f13ee76bf85cb182 
>   src/internal/evolve.cpp 93719127e75b4ba299f8d0a7fdeb4681818d30db 
>   src/master/http.cpp 1b74211acafc15cc0bcb24545ca4c2bacd79cb2d 
>   src/master/master.hpp 72c60ef74ce57119a97cf8305182340a13c58c42 
>   src/tests/api_tests.cpp 03e46cc5e0af8250ba36281e1293a1dc89f8e266 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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

(Updated June 17, 2016, 6:56 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Implement v1::master::Call::GET_TASKS.


Diffs (updated)
-

  include/mesos/v1/master/master.proto 5a6bbeb0f9b57d03803e366df32e04e9598e00ed 
  src/internal/evolve.hpp 5f3e267fa3a0420fcbed7be6f13ee76bf85cb182 
  src/internal/evolve.cpp 93719127e75b4ba299f8d0a7fdeb4681818d30db 
  src/master/http.cpp 1b74211acafc15cc0bcb24545ca4c2bacd79cb2d 
  src/master/master.hpp 72c60ef74ce57119a97cf8305182340a13c58c42 
  src/tests/api_tests.cpp 03e46cc5e0af8250ba36281e1293a1dc89f8e266 

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


Testing
---

make check on ubuntu 14.04


Thanks,

Jay Guo

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

> On June 17, 2016, 6:07 a.m., haosdent huang wrote:
> > src/master/http.cpp, line 1447
> > 
> >
> > How about change
> > ```
> >   message GetTasks {
> > optional string limit = 1;
> > optional uint32 offset = 2;
> > optional string order = 3;
> >   }
> > ```
> > 
> > to 
> > 
> > ```
> >   message GetTasks {
> > optional uint32 limit = 1;
> > optional uint32 offset = 2;
> > optional string order = 3;
> >   }
> > ```
> > 
> > And I not sure if change `optional string order = 3;` to enum would be 
> > better?

Thanks for your review!! This change is already made in 
https://reviews.apache.org/r/48401/

For order, I feel enum may overkill since it's essentially a bool. But I guess 
I could do another patch to refactor this and old impl, if you guys think it 
makes more sense.


> On June 17, 2016, 6:07 a.m., haosdent huang wrote:
> > src/master/http.cpp, line 1445
> > 
> >
> > I suggest to add 
> > ```
> > CHECK(call.has_get_tasks());
> > ```

It is already checked here: 
https://github.com/apache/mesos/blob/master/src/master/validation.cpp#L121


> On June 17, 2016, 6:07 a.m., haosdent huang wrote:
> > src/master/http.cpp, line 1457
> > 
> >
> > I suggest to change
> > ```
> > .then([=](const vector& tasks) -> Response {
> > ```
> > to 
> > ```
> > .then([contentTyle](const vector& tasks) -> Response {
> > ```
> > 
> > Refer to recent problems
> > https://issues.apache.org/jira/browse/MESOS-5587
> > and
> > https://issues.apache.org/jira/browse/MESOS-5629

OK


- Jay


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


On June 17, 2016, 5:47 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 17, 2016, 5:47 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master/master.proto 
> 5a6bbeb0f9b57d03803e366df32e04e9598e00ed 
>   src/internal/evolve.hpp 5f3e267fa3a0420fcbed7be6f13ee76bf85cb182 
>   src/internal/evolve.cpp 93719127e75b4ba299f8d0a7fdeb4681818d30db 
>   src/master/http.cpp 1b74211acafc15cc0bcb24545ca4c2bacd79cb2d 
>   src/master/master.hpp 72c60ef74ce57119a97cf8305182340a13c58c42 
>   src/tests/api_tests.cpp 03e46cc5e0af8250ba36281e1293a1dc89f8e266 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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




src/master/http.cpp (line 1445)


It is already checked here: 
https://github.com/apache/mesos/blob/master/src/master/validation.cpp#L121



src/master/http.cpp (line 1447)


Thanks for your review!! This change is already made in 
https://reviews.apache.org/r/48401/

For `order`, I feel enum may overkill since it's essentially a bool. But I 
guess I could do another patch to refactor this and old impl, if you guys think 
it makes more sense.



src/master/http.cpp (line 1457)


OK!


- Jay Guo


On June 17, 2016, 5:47 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 17, 2016, 5:47 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master/master.proto 
> 5a6bbeb0f9b57d03803e366df32e04e9598e00ed 
>   src/internal/evolve.hpp 5f3e267fa3a0420fcbed7be6f13ee76bf85cb182 
>   src/internal/evolve.cpp 93719127e75b4ba299f8d0a7fdeb4681818d30db 
>   src/master/http.cpp 1b74211acafc15cc0bcb24545ca4c2bacd79cb2d 
>   src/master/master.hpp 72c60ef74ce57119a97cf8305182340a13c58c42 
>   src/tests/api_tests.cpp 03e46cc5e0af8250ba36281e1293a1dc89f8e266 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[haosdent huang]

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




src/master/http.cpp (line 1442)


Should be
```
ContentType contentType)
```



src/master/master.hpp (line 1329)


```
ContentType& contentType)
```
should be

```
ContentType contentType)
```


- haosdent huang


On June 17, 2016, 5:47 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 17, 2016, 5:47 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master/master.proto 
> 5a6bbeb0f9b57d03803e366df32e04e9598e00ed 
>   src/internal/evolve.hpp 5f3e267fa3a0420fcbed7be6f13ee76bf85cb182 
>   src/internal/evolve.cpp 93719127e75b4ba299f8d0a7fdeb4681818d30db 
>   src/master/http.cpp 1b74211acafc15cc0bcb24545ca4c2bacd79cb2d 
>   src/master/master.hpp 72c60ef74ce57119a97cf8305182340a13c58c42 
>   src/tests/api_tests.cpp 03e46cc5e0af8250ba36281e1293a1dc89f8e266 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[haosdent huang]

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




src/master/http.cpp (line 1445)


I suggest to add 
```
CHECK(call.has_get_tasks());
```



src/master/http.cpp (line 1447)


How about change
```
  message GetTasks {
optional string limit = 1;
optional uint32 offset = 2;
optional string order = 3;
  }
```

to 

```
  message GetTasks {
optional uint32 limit = 1;
optional uint32 offset = 2;
optional string order = 3;
  }
```

And I not sure if change `optional string order = 3;` to enum would be 
better?



src/master/http.cpp (line 1457)


I suggest to change
```
.then([=](const vector& tasks) -> Response {
```
to 
```
.then([contentTyle](const vector& tasks) -> Response {
```

Refer to recent problems
https://issues.apache.org/jira/browse/MESOS-5587
and
https://issues.apache.org/jira/browse/MESOS-5629


- haosdent huang


On June 17, 2016, 5:47 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 17, 2016, 5:47 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master/master.proto 
> 5a6bbeb0f9b57d03803e366df32e04e9598e00ed 
>   src/internal/evolve.hpp 5f3e267fa3a0420fcbed7be6f13ee76bf85cb182 
>   src/internal/evolve.cpp 93719127e75b4ba299f8d0a7fdeb4681818d30db 
>   src/master/http.cpp 1b74211acafc15cc0bcb24545ca4c2bacd79cb2d 
>   src/master/master.hpp 72c60ef74ce57119a97cf8305182340a13c58c42 
>   src/tests/api_tests.cpp 03e46cc5e0af8250ba36281e1293a1dc89f8e266 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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

(Updated June 17, 2016, 5:47 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

rebase


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


Repository: mesos


Description
---

Implement v1::master::Call::GET_TASKS.


Diffs (updated)
-

  include/mesos/v1/master/master.proto 5a6bbeb0f9b57d03803e366df32e04e9598e00ed 
  src/internal/evolve.hpp 5f3e267fa3a0420fcbed7be6f13ee76bf85cb182 
  src/internal/evolve.cpp 93719127e75b4ba299f8d0a7fdeb4681818d30db 
  src/master/http.cpp 1b74211acafc15cc0bcb24545ca4c2bacd79cb2d 
  src/master/master.hpp 72c60ef74ce57119a97cf8305182340a13c58c42 
  src/tests/api_tests.cpp 03e46cc5e0af8250ba36281e1293a1dc89f8e266 

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


Testing
---

make check on ubuntu 14.04


Thanks,

Jay Guo

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Vinod Kone]

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




src/master/master.hpp (line 1317)


After the recent change, this should take unversioned master::Call.


- Vinod Kone


On June 9, 2016, 11:17 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 9, 2016, 11:17 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp 7f16cbda7da6c838648cca909368973e7298730b 
>   src/master/http.cpp 6e1bf9557a854a89fa9173223295816a9e114e7c 
>   src/master/master.hpp 2c45dab291a153b42809ab12e4252bf58559feeb 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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



Ping. I don't think it is necessary to devolve protobuf in this API. `_tasks()` 
only takes primitives: int32 and string.

- Jay Guo


On June 9, 2016, 11:17 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 9, 2016, 11:17 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp 7f16cbda7da6c838648cca909368973e7298730b 
>   src/master/http.cpp 6e1bf9557a854a89fa9173223295816a9e114e7c 
>   src/master/master.hpp 2c45dab291a153b42809ab12e4252bf58559feeb 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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

(Updated June 9, 2016, 11:17 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Implement v1::master::Call::GET_TASKS.


Diffs (updated)
-

  include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
  src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
  src/internal/evolve.cpp 7f16cbda7da6c838648cca909368973e7298730b 
  src/master/http.cpp 6e1bf9557a854a89fa9173223295816a9e114e7c 
  src/master/master.hpp 2c45dab291a153b42809ab12e4252bf58559feeb 
  src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 

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


Testing
---

make check on ubuntu 14.04


Thanks,

Jay Guo

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Vinod Kone]

> On June 9, 2016, 9:26 p.m., Vinod Kone wrote:
> > src/tests/api_tests.cpp, line 171
> > 
> >
> > What does this comment mean? I didn't follow.
> > 
> > Also, comments should start with a capital letter and end in a period.
> 
> Jay Guo wrote:
> This line of code is intended to invoke `set_has_get_tasks()` (private 
> method), which is to toggle `GET_TASKS` bit.
> ```
> inline void Call::set_has_get_tasks() {
>   _has_bits_[0] |= 0x0020u;
> }
> ```
> However this line looks a bit weird, so I left the comment there. If you 
> know any other way to toggle the bit, let me know.

I see. Just kill the comment. I think it is clear that you creating get_tasks() 
object.


- Vinod


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


On June 9, 2016, 12:49 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 9, 2016, 12:49 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp c255195b69f55f6429beed6c18a4a31b38528840 
>   src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
>   src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

> On June 9, 2016, 9:26 p.m., Vinod Kone wrote:
> > src/tests/api_tests.cpp, line 171
> > 
> >
> > What does this comment mean? I didn't follow.
> > 
> > Also, comments should start with a capital letter and end in a period.

This line of code is intended to invoke `set_has_get_tasks()` (private method), 
which is to toggle `GET_TASKS` bit.
```
inline void Call::set_has_get_tasks() {
  _has_bits_[0] |= 0x0020u;
}
```
However this line looks a bit weird, so I left the comment there. If you know 
any other way to toggle the bit, let me know.


- Jay


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


On June 9, 2016, 12:49 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 9, 2016, 12:49 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp c255195b69f55f6429beed6c18a4a31b38528840 
>   src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
>   src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Vinod Kone]

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




src/master/http.cpp (line 645)


you want to rebase this on top of haosdent's recent changes that I 
committed.



src/master/http.cpp (lines 1412 - 1415)


uncomment this after you rebase because `getTasks` will return 
http::Response() now.



src/tests/api_tests.cpp (line 164)


s/presents/is present/



src/tests/api_tests.cpp (line 171)


What does this comment mean? I didn't follow.

Also, comments should start with a capital letter and end in a period.



src/tests/api_tests.cpp (line 217)


give it a name, say "test"



src/tests/api_tests.cpp (lines 229 - 233)


do we really need this expectation? i think scheduler receiving the update 
should be enough to gurantee that master has already updated the state of the 
task?



src/tests/api_tests.cpp (line 246)


see above. kill this?



src/tests/api_tests.cpp (line 249)


ditto. see above.



src/tests/api_tests.cpp (line 262)


align this with first argument.

```
ASSERT_EQ(arg1,
  arg2);
```


- Vinod Kone


On June 9, 2016, 12:49 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 9, 2016, 12:49 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp c255195b69f55f6429beed6c18a4a31b38528840 
>   src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
>   src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

> On June 9, 2016, 6:20 p.m., haosdent huang wrote:
> > Hi, @guoger. Now the return type of RPC handlers become 
> > process::http::Response, may you rebase your patch?

Good job! working on it!


- Jay


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


On June 9, 2016, 12:49 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 9, 2016, 12:49 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp c255195b69f55f6429beed6c18a4a31b38528840 
>   src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
>   src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[haosdent huang]

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



Hi, @guoger. Now the return type of RPC handlers become 
process::http::Response, may you rebase your patch?

- haosdent huang


On June 9, 2016, 12:49 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 9, 2016, 12:49 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp c255195b69f55f6429beed6c18a4a31b38528840 
>   src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
>   src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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

(Updated June 9, 2016, 12:49 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Add comments for tests.


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


Repository: mesos


Description
---

Implement v1::master::Call::GET_TASKS.


Diffs (updated)
-

  include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
  src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
  src/internal/evolve.cpp c255195b69f55f6429beed6c18a4a31b38528840 
  src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
  src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
  src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 

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


Testing
---

make check on ubuntu 14.04


Thanks,

Jay Guo

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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

(Updated June 9, 2016, 12:01 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Implement v1::master::Call::GET_TASKS.


Diffs (updated)
-

  include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
  src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
  src/internal/evolve.cpp c255195b69f55f6429beed6c18a4a31b38528840 
  src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
  src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
  src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 

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


Testing
---

make check on ubuntu 14.04


Thanks,

Jay Guo

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

> On June 8, 2016, 5:34 p.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 1390-1411
> > 
> >
> > so this stuff is duplicated in the REST handler and this handler. i 
> > wonder if we can factor it out as well into a common function.
> > 
> > for example, can `_tasks()` take `limit`, `offset` and `order` as 
> > parameters and return a Future of Task* collection?

OK, something like:
```
Future _tasks(limit, offset, order, principle)
{
  return collect(...)
  .then(...)
}
```

`_tasks()` will need to take principle as an argument as well.


- Jay


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


On June 8, 2016, 7:18 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 8, 2016, 7:18 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
>   src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Vinod Kone]

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




src/master/http.cpp (lines 1390 - 1411)


so this stuff is duplicated in the REST handler and this handler. i wonder 
if we can factor it out as well into a common function.

for example, can `_tasks()` take `limit`, `offset` and `order` as 
parameters and return a Future of Task* collection?


- Vinod Kone


On June 8, 2016, 7:18 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 8, 2016, 7:18 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
>   src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [48400, 48401, 48046]

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 June 8, 2016, 7:18 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated June 8, 2016, 7:18 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
>   src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check on ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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

(Updated June 8, 2016, 7:18 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Implement v1::master::Call::GET_TASKS.


Diffs (updated)
-

  include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
  src/master/http.cpp 4b2d1386e1ecb447b597a432f9df9adaa5c3aa37 
  src/master/master.hpp 790da3ce686401c378ad9c62d497d60893c4ce41 
  src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 

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


Testing (updated)
---

make check on ubuntu 14.04


Thanks,

Jay Guo

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

> On May 30, 2016, 6:44 p.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 2420-2423
> > 
> >
> > this will have a performance regression for /tasks.
> > 
> > I would say for now don't touch tasks(). Just implement getTasks() from 
> > scratch, i.e., don't bother getting JSON::Object from `_tasks()` and 
> > evolving it.
> > 
> > Also v1::master::Response::GetTasks can just be
> > 
> > ```
> > message GetTasks {
> >   repeated Task tasks;
> > }
> > 
> > ```
> > 
> > because v1::Task is public now.
> 
> Vinod Kone wrote:
> Another option suggested by @mcypark is
> 
> ```
> if we have a vector getTasksHelper(...);
> 
> Future tasks(...) {
>   vector tasks = getTasksHelper(...);
> 
>   auto tasksWriter = [, ](...) { ... };
> 
>   return OK(jsonify(tasksWriter), ...);
> }
> 
> and have `getTasks` call `getTasksHelper` as well, except have it go 
> directly from `mesos::Task`s to `GetTasks::tasks`
> 
> even the offset logic could go into getTasksHelper()
> 
> ```

I don't see `message Task` in `mesos/include/mesos/v1/mesos.proto`...


- Jay


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


On May 30, 2016, 4:23 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated May 30, 2016, 4:23 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp 0fdda31bb2bdda46d23663379116fb7cc567055a 
>   src/internal/evolve.cpp 4f25f1a892b4834fc4d79b6fd1bd0edd458b7a58 
>   src/master/http.cpp c8d2f46d9e0ad8a99a6ebffc6a3d5d852cee0616 
>   src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 
>   src/tests/api_tests.cpp 0b6bfadae3a969b3cd1a48e5095e64c953193543 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Incomplete yet. A draft patch to show general idea how to refactor existing 
> endpoints that uses `jsonify` in OK().
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

> On June 7, 2016, 5:15 p.m., Vinod Kone wrote:
> > Any updates on this?

I'm working on it now and will submit a patch later today


- Jay


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


On May 30, 2016, 4:23 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated May 30, 2016, 4:23 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp 0fdda31bb2bdda46d23663379116fb7cc567055a 
>   src/internal/evolve.cpp 4f25f1a892b4834fc4d79b6fd1bd0edd458b7a58 
>   src/master/http.cpp c8d2f46d9e0ad8a99a6ebffc6a3d5d852cee0616 
>   src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 
>   src/tests/api_tests.cpp 0b6bfadae3a969b3cd1a48e5095e64c953193543 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Incomplete yet. A draft patch to show general idea how to refactor existing 
> endpoints that uses `jsonify` in OK().
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Vinod Kone]

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



Any updates on this?

- Vinod Kone


On May 30, 2016, 4:23 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated May 30, 2016, 4:23 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp 0fdda31bb2bdda46d23663379116fb7cc567055a 
>   src/internal/evolve.cpp 4f25f1a892b4834fc4d79b6fd1bd0edd458b7a58 
>   src/master/http.cpp c8d2f46d9e0ad8a99a6ebffc6a3d5d852cee0616 
>   src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 
>   src/tests/api_tests.cpp 0b6bfadae3a969b3cd1a48e5095e64c953193543 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Incomplete yet. A draft patch to show general idea how to refactor existing 
> endpoints that uses `jsonify` in OK().
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Vinod Kone]

> On May 30, 2016, 6:44 p.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 2420-2423
> > 
> >
> > this will have a performance regression for /tasks.
> > 
> > I would say for now don't touch tasks(). Just implement getTasks() from 
> > scratch, i.e., don't bother getting JSON::Object from `_tasks()` and 
> > evolving it.
> > 
> > Also v1::master::Response::GetTasks can just be
> > 
> > ```
> > message GetTasks {
> >   repeated Task tasks;
> > }
> > 
> > ```
> > 
> > because v1::Task is public now.

Another option suggested by @mcypark is

```
if we have a vector getTasksHelper(...);

Future tasks(...) {
  vector tasks = getTasksHelper(...);

  auto tasksWriter = [, ](...) { ... };

  return OK(jsonify(tasksWriter), ...);
}

and have `getTasks` call `getTasksHelper` as well, except have it go directly 
from `mesos::Task`s to `GetTasks::tasks`

even the offset logic could go into getTasksHelper()

```


- Vinod


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


On May 30, 2016, 4:23 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated May 30, 2016, 4:23 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp 0fdda31bb2bdda46d23663379116fb7cc567055a 
>   src/internal/evolve.cpp 4f25f1a892b4834fc4d79b6fd1bd0edd458b7a58 
>   src/master/http.cpp c8d2f46d9e0ad8a99a6ebffc6a3d5d852cee0616 
>   src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 
>   src/tests/api_tests.cpp 0b6bfadae3a969b3cd1a48e5095e64c953193543 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Incomplete yet. A draft patch to show general idea how to refactor existing 
> endpoints that uses `jsonify` in OK().
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [48046]

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 May 30, 2016, 4:23 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated May 30, 2016, 4:23 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp 0fdda31bb2bdda46d23663379116fb7cc567055a 
>   src/internal/evolve.cpp 4f25f1a892b4834fc4d79b6fd1bd0edd458b7a58 
>   src/master/http.cpp c8d2f46d9e0ad8a99a6ebffc6a3d5d852cee0616 
>   src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 
>   src/tests/api_tests.cpp 0b6bfadae3a969b3cd1a48e5095e64c953193543 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Incomplete yet. A draft patch to show general idea how to refactor existing 
> endpoints that uses `jsonify` in OK().
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Vinod Kone]

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




src/internal/evolve.hpp 


we leave 2 blank lines between outer elements.



src/master/http.cpp (line 484)


shift 2 spaces to the left.



src/master/http.cpp (line 1201)


can you fix the proto to make `limit` int32 type? that was a mistake on my 
part.



src/master/http.cpp (line 2419)


s/obj/object/



src/master/http.cpp (lines 2419 - 2422)


this will have a performance regression for /tasks.

I would say for now don't touch tasks(). Just implement getTasks() from 
scratch, i.e., don't bother getting JSON::Object from `_tasks()` and evolving 
it.

Also v1::master::Response::GetTasks can just be

```
message GetTasks {
  repeated Task tasks;
}

```

because v1::Task is public now.


- Vinod Kone


On May 30, 2016, 4:23 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48046/
> ---
> 
> (Updated May 30, 2016, 4:23 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5493
> https://issues.apache.org/jira/browse/MESOS-5493
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement v1::master::Call::GET_TASKS.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp 0fdda31bb2bdda46d23663379116fb7cc567055a 
>   src/internal/evolve.cpp 4f25f1a892b4834fc4d79b6fd1bd0edd458b7a58 
>   src/master/http.cpp c8d2f46d9e0ad8a99a6ebffc6a3d5d852cee0616 
>   src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 
>   src/tests/api_tests.cpp 0b6bfadae3a969b3cd1a48e5095e64c953193543 
> 
> Diff: https://reviews.apache.org/r/48046/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Incomplete yet. A draft patch to show general idea how to refactor existing 
> endpoints that uses `jsonify` in OK().
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 48046: Implement v1::master::Call::GET_TASKS.

[Jay Guo]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Implement v1::master::Call::GET_TASKS.


Diffs
-

  src/internal/evolve.hpp 0fdda31bb2bdda46d23663379116fb7cc567055a 
  src/internal/evolve.cpp 4f25f1a892b4834fc4d79b6fd1bd0edd458b7a58 
  src/master/http.cpp c8d2f46d9e0ad8a99a6ebffc6a3d5d852cee0616 
  src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 
  src/tests/api_tests.cpp 0b6bfadae3a969b3cd1a48e5095e64c953193543 

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


Testing
---

make check

Incomplete yet. A draft patch to show general idea how to refactor existing 
endpoints that uses `jsonify` in OK().


Thanks,

Jay Guo