Re: Review Request 48751: Implement GetState response for master API.

[haosdent huang]

> On June 23, 2016, 5:34 p.m., Zhitao Li wrote:
> > include/mesos/master/master.proto, lines 223-224
> > 
> >
> > Per previous conversation with Jay Guo, I'm inclined to make most 
> > fields as optional for now unless there is very strong reason.
> > 
> > I think upgrading from optional to required is possible if it's always 
> > set, but the otherway may not, right?

Got it, thank you for the explanation. XD


> On June 23, 2016, 5:34 p.m., Zhitao Li wrote:
> > include/mesos/v1/master/master.proto, lines 234-235
> > 
> >
> > We haven't introduced a `message Agent` in this patch yet.
> > 
> > There is some parallel conversation about unifying messages between 
> > various `Get*` methods in a differet patch from Jay Guo.
> > 
> > I'm inclined to drop these two fields in this patch until we have 
> > consensus about what to do with agents.
> > 
> > What do you think?

Agree, let me drop it as well.


- haosdent


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


On June 22, 2016, 5 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 22, 2016, 5 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `GetState` response for master API.
> 
> The response protobuf message and new function `_getState`
> will be reused for generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Zhitao Li]

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




include/mesos/master/master.proto (lines 223 - 224)


Per previous conversation with Jay Guo, I'm inclined to make most fields as 
optional for now unless there is very strong reason.

I think upgrading from optional to required is possible if it's always set, 
but the otherway may not, right?



include/mesos/v1/master/master.proto (lines 232 - 233)


Good point. Mistakenly thought that's not available in proto2. Will do.



include/mesos/v1/master/master.proto (lines 234 - 235)


We haven't introduced a `message Agent` in this patch yet.

There is some parallel conversation about unifying messages between various 
`Get*` methods in a differet patch from Jay Guo.

I'm inclined to drop these two fields in this patch until we have consensus 
about what to do with agents.

What do you think?


- Zhitao Li


On June 22, 2016, 5 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 22, 2016, 5 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `GetState` response for master API.
> 
> The response protobuf message and new function `_getState`
> will be reused for generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[haosdent huang]

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




include/mesos/master/master.proto (lines 223 - 224)


Should be `required`?



include/mesos/v1/master/master.proto (lines 232 - 233)


Should be `TimeInfo`?



include/mesos/v1/master/master.proto (lines 234 - 235)


These type should be `Agent`?


- haosdent huang


On June 22, 2016, 5 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 22, 2016, 5 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `GetState` response for master API.
> 
> The response protobuf message and new function `_getState`
> will be reused for generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Zhitao Li]

> On June 22, 2016, 6:29 a.m., Jay Guo wrote:
> > src/master/http.cpp, lines 1477-1500
> > 
> >
> > I feel we can extract this logic to a common function since it's being 
> > used by getTasks as well.
> 
> Zhitao Li wrote:
> Sounds good although I might want to do it in a follow up patch. 
> 
> How does a helper funciton `vector> 
> createApprovers(Optional principal, const 
> vector& actions)` sounds to you?
> 
> Jay Guo wrote:
> How about a step forward and let it return the Future of `collect`? 
> Something like:
> ```
> Future> createApprovers(Optional 
> principal, const vector& actions) {
>   std::list approvers;
>   ...
>   return collect(approvers);
> }
> ```

Created https://reviews.apache.org/r/49130 to fix in separate patch.


- Zhitao


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


On June 22, 2016, 5 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 22, 2016, 5 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `GetState` response for master API.
> 
> The response protobuf message and new function `_getState`
> will be reused for generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Jay Guo]

> On June 22, 2016, 6:29 a.m., Jay Guo wrote:
> > src/master/http.cpp, lines 1477-1500
> > 
> >
> > I feel we can extract this logic to a common function since it's being 
> > used by getTasks as well.
> 
> Zhitao Li wrote:
> Sounds good although I might want to do it in a follow up patch. 
> 
> How does a helper funciton `vector> 
> createApprovers(Optional principal, const 
> vector& actions)` sounds to you?

How about a step forward and let it return the Future of `collect`? Something 
like:
```
Future> createApprovers(Optional 
principal, const vector& actions) {
  std::list approvers;
  ...
  return collect(approvers);
}
```


- Jay


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


On June 22, 2016, 5 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 22, 2016, 5 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `GetState` response for master API.
> 
> The response protobuf message and new function `_getState`
> will be reused for generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Zhitao Li]

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

(Updated June 22, 2016, 5 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Make protobuf fields optional.


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


Repository: mesos


Description
---

Implement `GetState` response for master API.

The response protobuf message and new function `_getState`
will be reused for generating snapshot for Subscribe call in next patch.


Diffs (updated)
-

  include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
  include/mesos/v1/master/master.proto 11dfab318eb073908a9e302afa33b274fec63a16 
  src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
  src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
  src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 

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


Testing
---

New test in MasterAPITest.


Thanks,

Zhitao Li

Re: Review Request 48751: Implement GetState response for master API.

[Zhitao Li]

> On June 22, 2016, 6:29 a.m., Jay Guo wrote:
> > src/master/http.cpp, lines 1477-1500
> > 
> >
> > I feel we can extract this logic to a common function since it's being 
> > used by getTasks as well.

Sounds good although I might want to do it in a follow up patch. 

How does a helper funciton `vector> 
createApprovers(Optional principal, const 
vector& actions)` sounds to you?


- Zhitao


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


On June 22, 2016, 6:13 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 22, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `GetState` response for master API.
> 
> The response protobuf message and new function `_getState`
> will be reused for generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Zhitao Li]

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




include/mesos/master/master.proto (line 314)


Agreed. I think changing `required` mentioned in this patch to `optional` 
is good since the API here is still progressing.

@vinodkone, what's your opinion?


- Zhitao Li


On June 22, 2016, 6:13 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 22, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `GetState` response for master API.
> 
> The response protobuf message and new function `_getState`
> will be reused for generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Jay Guo]

> On June 22, 2016, 6:29 a.m., Jay Guo wrote:
> > include/mesos/master/master.proto, line 314
> > 
> >
> > I feel marking fields as `required` is troublesome and it's actually 
> > discouraged in protobuf: 
> > https://developers.google.com/protocol-buffers/docs/proto#specifying-field-rules
> > 
> > Before I raise it to broader audiences, I just wonder if we have a 
> > particular doing this?
> > 
> > @vinodkone

*particular reason


- Jay


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


On June 22, 2016, 6:13 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 22, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `GetState` response for master API.
> 
> The response protobuf message and new function `_getState`
> will be reused for generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Jay Guo]

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




include/mesos/master/master.proto (line 314)


I feel marking fields as `required` is troublesome and it's actually 
discouraged in protobuf: 
https://developers.google.com/protocol-buffers/docs/proto#specifying-field-rules

Before I raise it to broader audiences, I just wonder if we have a 
particular doing this?

@vinodkone



src/master/http.cpp (lines 1477 - 1500)


I feel we can extract this logic to a common function since it's being used 
by getTasks as well.


- Jay Guo


On June 22, 2016, 6:13 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 22, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement `GetState` response for master API.
> 
> The response protobuf message and new function `_getState`
> will be reused for generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> 11dfab318eb073908a9e302afa33b274fec63a16 
>   src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
>   src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
>   src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Zhitao Li]

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

(Updated June 22, 2016, 6:13 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

rebase and fix commit message line length.


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


Repository: mesos


Description (updated)
---

Implement `GetState` response for master API.

The response protobuf message and new function `_getState`
will be reused for generating snapshot for Subscribe call in next patch.


Diffs (updated)
-

  include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
  include/mesos/v1/master/master.proto 11dfab318eb073908a9e302afa33b274fec63a16 
  src/master/http.cpp 7daaf12a4086635bbc5aba5e3375c95e8899ac6e 
  src/master/master.hpp fe57878dc59637459d5c5cdae0be2aa159133fa4 
  src/tests/api_tests.cpp bf1a294f1fc3c8659c31115beee3876d4d0a45e2 

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


Testing
---

New test in MasterAPITest.


Thanks,

Zhitao Li

Re: Review Request 48751: Implement GetState response for master API.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [48751]

Failed command: ./support/apply-review.sh -n -r 48751

Error:
2016-06-22 05:47:25 URL:https://reviews.apache.org/r/48751/diff/raw/ 
[18537/18537] -> "48751.patch" [1]
Total errors found: 0
Checking 3 files
Error: No line in the commit message summary may exceed 72 characters.

Full log: https://builds.apache.org/job/mesos-reviewbot/13934/console

- Mesos ReviewBot


On June 21, 2016, 9:37 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 21, 2016, 9:37 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement GetState response for master API.
> 
> The response protobuf message and new function `_getState` will be reused for 
> generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> c5b57ddc6aca0a62fb28523aee2fa579212da0ed 
>   src/master/http.cpp 0196a79b4941ed6031afee8dfc35e11e23f07244 
>   src/master/master.hpp 2064f849edf6f78b009ecfcc4b15b6831834345c 
>   src/tests/api_tests.cpp 55df322f6a843e52d67bb09d3fe763739515602b 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Anand Mazumdar]

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




src/master/http.cpp (line 1462)


Wondering if we should not incur the performance penalty by evolving the 
response. We can just have a comment here about the reasoning for not doing so?

We already do something similar for `SlaveID` -> `AgentID` evolve for 
performance reasons?


- Anand Mazumdar


On June 21, 2016, 9:37 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 21, 2016, 9:37 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement GetState response for master API.
> 
> The response protobuf message and new function `_getState` will be reused for 
> generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> c5b57ddc6aca0a62fb28523aee2fa579212da0ed 
>   src/master/http.cpp 0196a79b4941ed6031afee8dfc35e11e23f07244 
>   src/master/master.hpp 2064f849edf6f78b009ecfcc4b15b6831834345c 
>   src/tests/api_tests.cpp 55df322f6a843e52d67bb09d3fe763739515602b 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Zhitao Li]

> On June 21, 2016, 1:29 a.m., Vinod Kone wrote:
> > include/mesos/v1/master.proto, lines 181-193
> > 
> >
> > How about defining *top level* `Framework` and maybe `Executor` 
> > message? I think it could be reused by GET_FRAMEWORKS and GET_AGENTS calls?
> > 
> > Infact, if Framework didn't contain `offers` we can move `Framework` 
> > and `Executor` to mesos.proto right next to `Task`.

Sounds good for top level in this file. Will do.


- Zhitao


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


On June 21, 2016, 9:37 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 21, 2016, 9:37 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement GetState response for master API.
> 
> The response protobuf message and new function `_getState` will be reused for 
> generating snapshot for Subscribe call in next patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
>   include/mesos/v1/master/master.proto 
> c5b57ddc6aca0a62fb28523aee2fa579212da0ed 
>   src/master/http.cpp 0196a79b4941ed6031afee8dfc35e11e23f07244 
>   src/master/master.hpp 2064f849edf6f78b009ecfcc4b15b6831834345c 
>   src/tests/api_tests.cpp 55df322f6a843e52d67bb09d3fe763739515602b 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Zhitao Li]

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

(Updated June 21, 2016, 9:37 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

backfill unversioned protobuf messages;
use inversion messages internally;
other review comments.
rebase.


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


Repository: mesos


Description (updated)
---

Implement GetState response for master API.

The response protobuf message and new function `_getState` will be reused for 
generating snapshot for Subscribe call in next patch.


Diffs (updated)
-

  include/mesos/master/master.proto 639fbd110df4aca1cf700cb9e455eecc110a7f66 
  include/mesos/v1/master/master.proto c5b57ddc6aca0a62fb28523aee2fa579212da0ed 
  src/master/http.cpp 0196a79b4941ed6031afee8dfc35e11e23f07244 
  src/master/master.hpp 2064f849edf6f78b009ecfcc4b15b6831834345c 
  src/tests/api_tests.cpp 55df322f6a843e52d67bb09d3fe763739515602b 

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


Testing
---

New test in MasterAPITest.


Thanks,

Zhitao Li

Re: Review Request 48751: Implement GetState response for master API.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [48751]

Failed command: ./support/apply-review.sh -n -r 48751

Error:
2016-06-21 05:05:43 URL:https://reviews.apache.org/r/48751/diff/raw/ 
[20562/20562] -> "48751.patch" [1]
error: include/mesos/v1/master.proto: does not exist in index
error: patch failed: include/mesos/v1/mesos.proto:1961
error: include/mesos/v1/mesos.proto: patch does not apply
error: patch failed: src/internal/evolve.hpp:56
error: src/internal/evolve.hpp: patch does not apply
error: patch failed: src/master/http.cpp:617
error: src/master/http.cpp: patch does not apply
error: patch failed: src/tests/api_tests.cpp:36
error: src/tests/api_tests.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/13916/console

- Mesos ReviewBot


On June 15, 2016, 8:52 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 15, 2016, 8:52 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement GetState response for master API, which should be functional 
> equivalent (but not necessarily fully compatible on json with `/state` 
> endpoint).
> 
> The response protobuf message and new function `_getState` will be reused for 
> generating snapshot for `Subscribe` call in next patch.
> 
> Also copied `Task` message to V1 API.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   include/mesos/v1/mesos.proto 39967fa09d2774d564f3df28277edea8ebcfb50d 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp 7f16cbda7da6c838648cca909368973e7298730b 
>   src/master/http.cpp db625f0d656f207a89fcc14b18ae2fc31d30e673 
>   src/master/master.hpp a0944ddccd3a4b33458cd2489bb5fcdbbdc55720 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Vinod Kone]

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




include/mesos/v1/master.proto (lines 181 - 193)


How about defining *top level* `Framework` and maybe `Executor` message? I 
think it could be reused by GET_FRAMEWORKS and GET_AGENTS calls?

Infact, if Framework didn't contain `offers` we can move `Framework` and 
`Executor` to mesos.proto right next to `Task`.



include/mesos/v1/mesos.proto (line 1303)


can you rebase? v1::Task is now committed.



src/internal/evolve.hpp (line 59)


this should already be there.



src/master/http.cpp (line 1386)


this should take unversioned call now.



src/master/http.cpp (lines 1392 - 1393)


no need for this. kill it.



src/master/http.cpp (lines 1397 - 1401)


Use unversioned master::Response and do evolve just before `return`ing. 
This is the pattern used by other handlers now.

```
return OK(serialize(contentType, evolve(response),
  stringify(contentType);
```



src/master/http.cpp (line 1410)


indent.



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


kill it.



src/master/http.cpp (line 1522)


put this on the line above

```
foreachpair(arg1,
arg2,
arg3)
```



src/master/http.cpp (line 1547)


indentation.



src/tests/api_tests.cpp (line 244)


It's not clear what the type of `framework` is, so I would recommend 
calling out the type explicitly instead of using `auto`.

also add a new line before this.



src/tests/api_tests.cpp (line 292)


see above.



src/tests/api_tests.cpp (line 328)


see abovwe.


- Vinod Kone


On June 15, 2016, 8:52 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> ---
> 
> (Updated June 15, 2016, 8:52 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implement GetState response for master API, which should be functional 
> equivalent (but not necessarily fully compatible on json with `/state` 
> endpoint).
> 
> The response protobuf message and new function `_getState` will be reused for 
> generating snapshot for `Subscribe` call in next patch.
> 
> Also copied `Task` message to V1 API.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   include/mesos/v1/mesos.proto 39967fa09d2774d564f3df28277edea8ebcfb50d 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp 7f16cbda7da6c838648cca909368973e7298730b 
>   src/master/http.cpp db625f0d656f207a89fcc14b18ae2fc31d30e673 
>   src/master/master.hpp a0944ddccd3a4b33458cd2489bb5fcdbbdc55720 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> ---
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 48751: Implement GetState response for master API.

[Zhitao Li]

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

(Updated June 15, 2016, 8:52 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Incorrectly includes something from next patch. Also fix slave->agent comment.


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


Repository: mesos


Description (updated)
---

Implement GetState response for master API, which should be functional 
equivalent (but not necessarily fully compatible on json with `/state` 
endpoint).

The response protobuf message and new function `_getState` will be reused for 
generating snapshot for `Subscribe` call in next patch.

Also copied `Task` message to V1 API.


Diffs (updated)
-

  include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
  include/mesos/v1/mesos.proto 39967fa09d2774d564f3df28277edea8ebcfb50d 
  src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
  src/internal/evolve.cpp 7f16cbda7da6c838648cca909368973e7298730b 
  src/master/http.cpp db625f0d656f207a89fcc14b18ae2fc31d30e673 
  src/master/master.hpp a0944ddccd3a4b33458cd2489bb5fcdbbdc55720 
  src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 

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


Testing
---

New test in MasterAPITest.


Thanks,

Zhitao Li