Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@mesos.apache.org
Content-Type: multipart/alternative;
 boundary="===============7305915481166773809=="
MIME-Version: 1.0
Subject: Re: Review Request 44255: Add metrics for {RESERVE, UNRESERVE,
 CREATE,
 DESTROY} offer operation.
From: Greg Mann <greg@mesosphere.io>
To: Jie Yu <yujie.jay@gmail.com>, Greg Mann <greg@mesosphere.io>,
 Guangya Liu <gyliu513@gmail.com>, haosdent huang <haosdent@gmail.com>
Cc: Mesos ReviewBot <reviews@mesos.apache.org>, fan du <fan.du@intel.com>,
 mesos <reviews@mesos.apache.org>
Date: Wed, 09 Mar 2016 18:01:33 -0000
Message-ID: <20160309180133.3657.32901@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Greg Mann <noreply@reviews.apache.org>
References: <20160307224317.8638.48520@reviews.apache.org>
In-Reply-To: <20160307224317.8638.48520@reviews.apache.org>
Reply-To: Greg Mann <greg@mesosphere.io>

--===============7305915481166773809==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On March 7, 2016, 10:43 p.m., Jie Yu wrote:
> > src/master/master.cpp, line 2846
> > <https://reviews.apache.org/r/44255/diff/2/?file=1278717#file1278717line2846>
> >
> >     I looked weird to me that we increase the metrics for reserve resources in 'authorizeXXX' function. Can you do that in the callers of authorizeXXX function instead?
> 
> fan du wrote:
>     Thanks for the reviewing :)
>     It intended to reduce code duplication, do the statistics counting here in the common path from both http endpoint and normal framework RECEIVE message. If you insist, I'm willing to do it seprately.

I think Jie's reasoning is that semantically, it is strange for the `authorizeXXX` function to have the side-effect of incrementing the metrics. I missed this in my reviews, but on further consideration this seems like a case where duplicating code is worth it in order to maintain the proper separation of concerns in the `authorizeXXX` functions.


- Greg


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


On March 3, 2016, 5:40 a.m., fan du wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44255/
> -----------------------------------------------------------
> 
> (Updated March 3, 2016, 5:40 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/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:
> 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
> 
>


--===============7305915481166773809==--