Re: Review Request 37024: Add an endpoint that exposes component version.

[haosdent huang]

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

(Updated Aug. 16, 2015, 5:37 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

Add an endpoint that exposes component version.


Diffs (updated)
-

  include/mesos/executor.hpp 72eca97dd84fb1300b37764a3ef3a57fb5e676c2 
  include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
  src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
  src/exec/exec.cpp 31e0c2f17a9092d18285828111d27628fb07bc02 
  src/local/local.cpp 4d98bf23705027f3ba0cbb571289f21b288fe7db 
  src/master/main.cpp bafc605d6c20bd264b932e44ee80373a3f692734 
  src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
  src/slave/main.cpp 364dc7fc7ab2e3cef01aea7267dafa014b60e2b9 
  src/version/version_info.hpp PRE-CREATION 
  src/version/version_info.cpp PRE-CREATION 

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


Testing (updated)
---

Manual test result:

```
$ curl http://localhost:5050/version 2>/dev/null|jq .

{
  "version": "0.24.0",
  "build_user": "haosdent",
  "build_time": 1439702338,
  "build_date": "2015-08-16 13:18:58"
}
```


Thanks,

haosdent huang

Re: Review Request 37024: Add an endpoint that exposes component version.

[haosdent huang]

> On Aug. 3, 2015, 7:37 p.m., Ben Mahler wrote:
> > src/master/http.cpp, lines 526-542
> > 
> >
> > Could you include these fields as well? They capture "version" / 
> > "build" information. Once you do, perhaps it is worth adding a 
> > '`JSON::Object version()`' helper in common/http.hpp.
> 
> Marco Massenzio wrote:
> great point.
> also wondering if, at this point, it would make sense to have a Protobuf 
> `VersionInfo` (that we could also use to replace the `version` string in 
> `MasterInfo`) which can then be used everywhere (and gets auto-converted to 
> JSON)?

Thank you @marco, it is OK for me. What's @bmahler opinion? :-P


- haosdent


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


On Aug. 2, 2015, 10:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 2, 2015, 10:16 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f 
>   src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc 
>   src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 
>   src/slave/http.cpp b0fe5f520dfca156548ba8c436d42fc432223f3d 
>   src/slave/slave.hpp 41d09497be313819a9c78361b8595f6f26dc8460 
>   src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/slave(1)/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> ```
> $ curl http://localhost:5050/master/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37024: Add an endpoint that exposes component version.

[haosdent huang]

> On Aug. 8, 2015, 9:19 a.m., Marco Massenzio wrote:
> > src/master/http.cpp, line 1185
> > 
> >
> > can you please double-check this is consistent with what we add to 
> > `MasterInfo` in the master's `initialize()` method?
> > I think it is, but not sure.
> > (sorry, I don't have the code at hand on this Mac)

In master's `initialize()` method,
```cpp
  info_.set_version(MESOS_VERSION);
```

So I think they are same. Should I change to use `info.get_version` here?


- haosdent


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


On Aug. 2, 2015, 10:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 2, 2015, 10:16 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f 
>   src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc 
>   src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 
>   src/slave/http.cpp b0fe5f520dfca156548ba8c436d42fc432223f3d 
>   src/slave/slave.hpp 41d09497be313819a9c78361b8595f6f26dc8460 
>   src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/slave(1)/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> ```
> $ curl http://localhost:5050/master/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37024: Add an endpoint that exposes component version.

[Marco Massenzio]

> On Aug. 3, 2015, 7:37 p.m., Ben Mahler wrote:
> > src/master/http.cpp, lines 526-542
> > 
> >
> > Could you include these fields as well? They capture "version" / 
> > "build" information. Once you do, perhaps it is worth adding a 
> > '`JSON::Object version()`' helper in common/http.hpp.

great point.
also wondering if, at this point, it would make sense to have a Protobuf 
`VersionInfo` (that we could also use to replace the `version` string in 
`MasterInfo`) which can then be used everywhere (and gets auto-converted to 
JSON)?


- Marco


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


On Aug. 2, 2015, 10:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 2, 2015, 10:16 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f 
>   src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc 
>   src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 
>   src/slave/http.cpp b0fe5f520dfca156548ba8c436d42fc432223f3d 
>   src/slave/slave.hpp 41d09497be313819a9c78361b8595f6f26dc8460 
>   src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/slave(1)/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> ```
> $ curl http://localhost:5050/master/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37024: Add an endpoint that exposes component version.

[Marco Massenzio]

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



src/master/http.cpp (line 1185)


can you please double-check this is consistent with what we add to 
`MasterInfo` in the master's `initialize()` method?
I think it is, but not sure.
(sorry, I don't have the code at hand on this Mac)


- Marco Massenzio


On Aug. 2, 2015, 10:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 2, 2015, 10:16 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f 
>   src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc 
>   src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 
>   src/slave/http.cpp b0fe5f520dfca156548ba8c436d42fc432223f3d 
>   src/slave/slave.hpp 41d09497be313819a9c78361b8595f6f26dc8460 
>   src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/slave(1)/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> ```
> $ curl http://localhost:5050/master/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37024: Add an endpoint that exposes component version.

[haosdent huang]

> On Aug. 3, 2015, 7:37 p.m., Ben Mahler wrote:
> > Thanks! Let's include the git / build information as well.
> > 
> > Another question, how are you planning to add this to the scheduler driver 
> > and executor driver? In these cases, it is likely better to create a 
> > 'Version' Process with process id "version" that routes an endpoint at "/" 
> > (so just "/version" should route to this). If we go with this approach, we 
> > just have to start a 'Version' Process from the master/slave entrypoints 
> > and the drivers' initialization.

Thank you very much. Let me use VersionProcess


- haosdent


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


On Aug. 2, 2015, 10:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 2, 2015, 10:16 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f 
>   src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc 
>   src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 
>   src/slave/http.cpp b0fe5f520dfca156548ba8c436d42fc432223f3d 
>   src/slave/slave.hpp 41d09497be313819a9c78361b8595f6f26dc8460 
>   src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/slave(1)/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> ```
> $ curl http://localhost:5050/master/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37024: Add an endpoint that exposes component version.

[Ben Mahler]

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


Thanks! Let's include the git / build information as well.

Another question, how are you planning to add this to the scheduler driver and 
executor driver? In these cases, it is likely better to create a 'Version' 
Process with process id "version" that routes an endpoint at "/" (so just 
"/version" should route to this). If we go with this approach, we just have to 
start a 'Version' Process from the master/slave entrypoints and the drivers' 
initialization.


src/master/http.cpp (lines 526 - 542)


Could you include these fields as well? They capture "version" / "build" 
information. Once you do, perhaps it is worth adding a '`JSON::Object 
version()`' helper in common/http.hpp.


- Ben Mahler


On Aug. 2, 2015, 10:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 2, 2015, 10:16 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f 
>   src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc 
>   src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 
>   src/slave/http.cpp b0fe5f520dfca156548ba8c436d42fc432223f3d 
>   src/slave/slave.hpp 41d09497be313819a9c78361b8595f6f26dc8460 
>   src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/slave(1)/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> ```
> $ curl http://localhost:5050/master/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37024: Add an endpoint that exposes component version.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37024]

All tests passed.

- Mesos ReviewBot


On Aug. 2, 2015, 10:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37024/
> ---
> 
> (Updated Aug. 2, 2015, 10:16 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1841
> https://issues.apache.org/jira/browse/MESOS-1841
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an endpoint that exposes component version.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 3772e39015a22655dcad00ad844dc5ddc90db43f 
>   src/master/master.hpp ea18c4e0bb0743747401b9cd5ea14ae9b56ae3cc 
>   src/master/master.cpp 351a3c2b5f551ad065682cea601d2436258e4544 
>   src/slave/http.cpp b0fe5f520dfca156548ba8c436d42fc432223f3d 
>   src/slave/slave.hpp 41d09497be313819a9c78361b8595f6f26dc8460 
>   src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 
> 
> Diff: https://reviews.apache.org/r/37024/diff/
> 
> 
> Testing
> ---
> 
> Manual test result:
> 
> ```
> $ curl http://localhost:5050/slave(1)/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> ```
> $ curl http://localhost:5050/master/version.json 2>/dev/null|jq .
> {
>   "version": "0.24.0"
> }
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>