You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Niklas Nielsen <ni...@qni.dk> on 2015/03/03 20:01:10 UTC
Re: Review Request 30074: Added "max allowed age" to Slave state.json
endpoint
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30074/#review75009
-----------------------------------------------------------
src/slave/metrics.cpp
<https://reviews.apache.org/r/30074/#comment121931>
Let's preserve the ordering you have above :)
src/slave/metrics.cpp
<https://reviews.apache.org/r/30074/#comment121932>
Same here
src/slave/slave.hpp
<https://reviews.apache.org/r/30074/#comment121933>
Same as above.
src/slave/slave.cpp
<https://reviews.apache.org/r/30074/#comment121928>
Even though age() returns a Duration, it's not clear from the calling code. We could make it explicit by introducing a CHECK()
We could also have a local variable with the age which we set just before the delay so we don't have to .get().
Bot no biggies, but to catch if someone changes the return type of age()
src/slave/slave.cpp
<https://reviews.apache.org/r/30074/#comment121930>
How about using Slave::age(0) i.e. Assume that tasks don't take up any space yet. That will be closer to the number users will get after it has been measured.
src/slave/slave.cpp
<https://reviews.apache.org/r/30074/#comment121926>
No need for your else clause when you end the previous block with a return
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/30074/#comment121923>
Why this change?
- Niklas Nielsen
On Feb. 19, 2015, 1:03 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30074/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 1:03 a.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Bugs: mesos-2229
> https://issues.apache.org/jira/browse/mesos-2229
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds and entry `max_allowed_age_secs` to the response of the `state.json` endpoint in the slave.
>
> The max allowed age value is available only if it is set, which depends on the flag `--disk_watch_interval` which by default runs every minute (So it will only be available after the first minute running).
>
> Modifies the unit test `SlaveTest.StateEndpoint` so it takes into account the new entry.
>
>
> Diffs
> -----
>
> src/slave/metrics.hpp e2ce845ec2229e1e6e71c710022551bec9d15029
> src/slave/metrics.cpp ee610b8a7d352c20e8a91bb86c8aa2c1f011e910
> src/slave/slave.hpp 7a399f6df50c69b7e1e12d74f076fa57b6edb1b3
> src/slave/slave.cpp ec7ec1356e745bb07484ae1755c9183b038043b3
> src/tests/slave_tests.cpp 21709e2858d91a2746e0343786f73dec22735da6
>
> Diff: https://reviews.apache.org/r/30074/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 30074: Added "max allowed age" to Slave state.json
endpoint
Posted by Alexander Rojas <al...@mesosphere.io>.
> On March 3, 2015, 8:01 p.m., Niklas Nielsen wrote:
> > src/slave/slave.cpp, line 3850
> > <https://reviews.apache.org/r/30074/diff/9/?file=868894#file868894line3850>
> >
> > How about using Slave::age(0) i.e. Assume that tasks don't take up any space yet. That will be closer to the number users will get after it has been measured.
I used `Slave::age(0)` in a previous version of the patch, because it looked logical. However someone complained about how arbitrary that value was and requested to use the user given value. I did the change however, but the discussion may arise again.
> On March 3, 2015, 8:01 p.m., Niklas Nielsen wrote:
> > src/slave/slave.cpp, line 3452
> > <https://reviews.apache.org/r/30074/diff/9/?file=868894#file868894line3452>
> >
> > Even though age() returns a Duration, it's not clear from the calling code. We could make it explicit by introducing a CHECK()
> >
> > We could also have a local variable with the age which we set just before the delay so we don't have to .get().
> >
> > Bot no biggies, but to catch if someone changes the return type of age()
I'm not sure what you mean here. How would that check look? I did however added a local variable.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30074/#review75009
-----------------------------------------------------------
On March 4, 2015, 5:21 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30074/
> -----------------------------------------------------------
>
> (Updated March 4, 2015, 5:21 p.m.)
>
>
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
>
>
> Bugs: mesos-2229
> https://issues.apache.org/jira/browse/mesos-2229
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds and entry `max_allowed_age_secs` to the response of the `state.json` endpoint in the slave.
>
> The max allowed age value is available only if it is set, which depends on the flag `--disk_watch_interval` which by default runs every minute (So it will only be available after the first minute running).
>
> Modifies the unit test `SlaveTest.StateEndpoint` so it takes into account the new entry.
>
>
> Diffs
> -----
>
> src/slave/metrics.hpp 0390461968112cac9ed84ec3cc23b264e8b4e9d7
> src/slave/metrics.cpp c32b4d8a1ff3bc5e5f5ca21eb78a036c9ea1a813
> src/slave/slave.hpp d476d850b195e917c5fcb5c82f69863b4f85e4b1
> src/slave/slave.cpp 2d52ea0c440fd530174b0e44a59c8ae68fa2616c
> src/tests/slave_tests.cpp 27b3410d25256951d685fa2a7f6d425b3e80709f
>
> Diff: https://reviews.apache.org/r/30074/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>