mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 70117: Added unit tests for offer operation feedback metrics.
Date Wed, 13 Mar 2019 16:02:11 GMT


> On March 13, 2019, 12:29 a.m., Joseph Wu wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 3832-3845 (original), 3833-3846 (patched)
> > <https://reviews.apache.org/r/70117/diff/4/?file=2130951#file2130951line3833>
> >
> >     In cases like this, it would be more appropriate to use `Metrics()` since we'd
otherwise make multiple calls to `/metrics/snapshot` for each metric.
> >     
> >     Alternatively, you could add a verion of the `metricsEqual` which takes a map.
 The downside of a helper is that reporting which metric(s) don't match would be harder.
> >     
> >     ... also you forgot to delete `snapshot = Metrics();` ;)

I actually thought about your first point when implementing this, but thought that readability
would be more important than shaving off a few milliseconds from a test case.

Regarding the `snapshot`, you are of course right and I should have read the comment before
pushing the next revision ;)


- Benno


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


On March 13, 2019, 4:01 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70117/
> -----------------------------------------------------------
> 
> (Updated March 13, 2019, 4:01 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
>     https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This adds a set of checks to verify the metrics introduced
> in the previous commit are working as intended.
> 
> 
> Diffs
> -----
> 
>   src/tests/agent_operation_feedback_tests.cpp 5a8f54c7c53272e90ed5fa6366e8154cedf1375f

>   src/tests/api_tests.cpp f241064dc8597972299a424958e759588f7e4fd2 
>   src/tests/master_slave_reconciliation_tests.cpp 002be27bf0731e2dba89376911117b347cd1dd0a

>   src/tests/master_tests.cpp 5a926831734e6acf0388a63dac3ea3559b44a6a9 
>   src/tests/operation_reconciliation_tests.cpp 6a815ad694e2a608ce324715c920833f825793a0

>   src/tests/persistent_volume_endpoints_tests.cpp 40d7e6a30c9c11eb84f4bd5aca92cfcecb3e0eb7

>   src/tests/reservation_endpoints_tests.cpp b1897592797c40574de7995b2335f2b4bc5fc699

>   src/tests/scheduler_tests.cpp 5fb696061248c877bfa86727f146051aee26cb58 
>   src/tests/slave_tests.cpp 5ee5609af0861e9aecf02a5eaefafe137bd9b843 
>   src/tests/storage_local_resource_provider_tests.cpp 7945384867f26fa15dc734a235ae509d5d6d350f

> 
> 
> Diff: https://reviews.apache.org/r/70117/diff/6/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message