You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2013/02/04 22:15:43 UTC
Re: Review Request: Resource Monitoring 1: Add name to ExecutorInfo.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9089/#review16065
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/9089/#comment34367>
You should always be using increasing numbers to identify new fields. It's highly likely that '2' was used before. While it's less likely that there is still a version of the code with '2' being run when we (or someone else) upgrades, there's not reason to run that risk. Likewise, I'd move 'name' down below resources since it's optional and the others are required (or at least 'framework_id' should be required, see comment). Plus it's nice to keep things related together (the two IDs in this case). Finally, let's update the comment to explain the value of 'name', in particular that the aggregate resource usage for the executor and all tasks/threads/processes underneath it will be exposed via JSON on the slave so that users can import that information into their own TSDB for monitoring/observability.
src/examples/long_lived_framework.cpp
<https://reviews.apache.org/r/9089/#comment34368>
Just because I'm so pedantic, I would update the order you set the name to match the order in the proto file (I know it matches now, but given my comment above, if it moves, you can update here and other spots too).
src/slave/http.cpp
<https://reviews.apache.org/r/9089/#comment34369>
Does the "blockers before 1.0" JIRA include switching to your protobuf -> JSON converter? Because it should!
- Benjamin Hindman
On Jan. 28, 2013, 10:41 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9089/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2013, 10:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This adds an executor name into ExecutorInfo.
>
> This name is useful to humans and frameworks. In particular, this proves useful at Twitter to further identify executors for resource monitoring.
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
> src/examples/long_lived_framework.cpp 04ac678387dd78104b5d42fa1f7b5de1849b0701
> src/examples/python/test_framework.py 269f532733ab82378bc1bd643107e6f9f2945d8b
> src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
> src/master/http.cpp 790961d6890841c456485491516d1991fe35ec16
> src/slave/http.cpp b94197013ac0d5dd95d6dabb44812905a123184a
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/webui/master/static/slave_executor.html bd06e69b3c99a0a44993c31429d877914b644a1d
> src/webui/master/static/slave_framework.html c66150ecb794300239c92ceaf684bfd3f1cb007e
>
> Diff: https://reviews.apache.org/r/9089/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request: Resource Monitoring 1: Add name to ExecutorInfo.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 4, 2013, 9:15 p.m., Benjamin Hindman wrote:
> > include/mesos/mesos.proto, line 130
> > <https://reviews.apache.org/r/9089/diff/2/?file=252480#file252480line130>
> >
> > You should always be using increasing numbers to identify new fields. It's highly likely that '2' was used before. While it's less likely that there is still a version of the code with '2' being run when we (or someone else) upgrades, there's not reason to run that risk. Likewise, I'd move 'name' down below resources since it's optional and the others are required (or at least 'framework_id' should be required, see comment). Plus it's nice to keep things related together (the two IDs in this case). Finally, let's update the comment to explain the value of 'name', in particular that the aggregate resource usage for the executor and all tasks/threads/processes underneath it will be exposed via JSON on the slave so that users can import that information into their own TSDB for monitoring/observability.
Using 9 now, moved name down, and added a comment.
> On Feb. 4, 2013, 9:15 p.m., Benjamin Hindman wrote:
> > src/slave/http.cpp, line 149
> > <https://reviews.apache.org/r/9089/diff/2/?file=252486#file252486line149>
> >
> > Does the "blockers before 1.0" JIRA include switching to your protobuf -> JSON converter? Because it should!
Added to that ticket.
> On Feb. 4, 2013, 9:15 p.m., Benjamin Hindman wrote:
> > src/examples/long_lived_framework.cpp, line 170
> > <https://reviews.apache.org/r/9089/diff/2/?file=252482#file252482line170>
> >
> > Just because I'm so pedantic, I would update the order you set the name to match the order in the proto file (I know it matches now, but given my comment above, if it moves, you can update here and other spots too).
done here and elsewhere
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9089/#review16065
-----------------------------------------------------------
On Jan. 28, 2013, 10:41 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9089/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2013, 10:41 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Description
> -------
>
> This adds an executor name into ExecutorInfo.
>
> This name is useful to humans and frameworks. In particular, this proves useful at Twitter to further identify executors for resource monitoring.
>
>
> This addresses bug MESOS-324.
> https://issues.apache.org/jira/browse/MESOS-324
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/examples/java/TestFramework.java 8417394487a80b439e7d9897c83f0b2c1eb17ff4
> src/examples/long_lived_framework.cpp 04ac678387dd78104b5d42fa1f7b5de1849b0701
> src/examples/python/test_framework.py 269f532733ab82378bc1bd643107e6f9f2945d8b
> src/examples/test_framework.cpp b9ab692414e4df64f176fc1ecd05f24ff089bde0
> src/master/http.cpp 790961d6890841c456485491516d1991fe35ec16
> src/slave/http.cpp b94197013ac0d5dd95d6dabb44812905a123184a
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/webui/master/static/slave_executor.html bd06e69b3c99a0a44993c31429d877914b644a1d
> src/webui/master/static/slave_framework.html c66150ecb794300239c92ceaf684bfd3f1cb007e
>
> Diff: https://reviews.apache.org/r/9089/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>