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



Thanks for the edits! A few comments below.


src/master/master.cpp (line 3027)
<https://reviews.apache.org/r/44255/#comment185461>

    You could move your incrementing statements here to be more consistent with 
the existing code.



src/master/master.cpp (line 3214)
<https://reviews.apache.org/r/44255/#comment185457>

    Remove the space after ->



src/master/http.cpp (line 787)
<https://reviews.apache.org/r/44255/#comment185462>

    This increment statement occurs after some invalidation logic, and directly 
before the authorization call; is this precisely where we want to be 
incrementing? We end up not counting invalid operations, but counting 
unauthorized operations.
    
    Looking through the other metrics code in master.cpp, it seems we aren't 
entirely consistent with respect to when we increment the `messages_XXX` 
metrics. We increment them both before and after operation validation, for 
example.
    
    In master.cpp, there are some existing `messages_XXX` metrics in the 
`accept()` function which get incremented together; those occur before 
validation and before authorization. To be consistent, you should probably do 
the same here. Currently, the metric gets incremented after `validate()` is 
called.
    
    We should create a separate JIRA for making all of the messages metrics 
consistent with respect to when they get incremented.


In a separate patch, could you add documentation for these metrics to 
docs/monitoring.md?

- Greg Mann


On March 10, 2016, 2:42 a.m., fan du wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44255/
> -----------------------------------------------------------
> 
> (Updated March 10, 2016, 2:42 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Guangya Liu, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4492
>     https://issues.apache.org/jira/browse/MESOS-4492
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add metrics for {RESERVE, UNRESERVE, CREATE, DESTROY} offer operation.
> 
> 
> Diffs
> -----
> 
>   docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
>   src/master/http.cpp 950206baf7f3a1cccdc49d810126473966d8d021 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 
>   src/master/metrics.cpp 30c091198a8fdd6d6a957a351dc37d3dae7788e4 
>   src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
> 
> Diff: https://reviews.apache.org/r/44255/diff/
> 
> 
> Testing
> -------
> 
> ChangLog:
> 
> v3:
>   - Move the couting out of common code path to http endpoint and master 
> accept call separately to reflect its logic.
> 
> v2:
>   - Documenting those metrics
>   - Add test code for MetricsTest as suggested by Guangya
>   - post-review.py does not update original 
> RR(https://reviews.apache.org/r/44058/), but only create a new one even if I 
> rebased.
> 
> 
> Tests:
> 1. make check GTEST_FILTER="MetricsTest.Master" on Centos-7 
> (3.10.0-123.el7.x86_640)
> 
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from MetricsTest
> [ RUN      ] MetricsTest.Master
> [       OK ] MetricsTest.Master (211 ms)
> [----------] 1 test from MetricsTest (211 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (244 ms total)
> [  PASSED  ] 1 test
> 
> 2. Verify its functionality with 'reserve' http endpoint as an test case
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot | python -mjson.tool | 
> grep reserve
>     "master/messages_reserve_resource": 0.0,
>     "master/messages_unreserve_resource": 0.0,
> 
> 
> # curl -i -d slaveId=6250553a-2f39-4a92-9073-4618d130f433-S1  -d resources='[ 
> { "name": "cpus", "type": "SCALAR","scalar": { "value": 1 
> },"reservation":{"principal": "XiaoHaHa"}}  ]' -X POST  
> ipdc02-kvm-guest2:5050/master/reserve
> HTTP/1.1 200 OK
> Date: Fri, 26 Feb 2016 19:59:01 GMT
> Content-Length: 0
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot  | python -mjson.tool | 
> grep reserve
>     "master/messages_reserve_resource": 1.0,
>     "master/messages_unreserve_resource": 0.0,
> 
> 
> Thanks,
> 
> fan du
> 
>

Reply via email to