Re: Review Request 27531: Update Master metrics to match task source and reason scheme.

[Vinod Kone]

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


Looks great. Just need some better naming. I suggested one. Feel free to think 
of better ones.


src/master/master.cpp
<https://reviews.apache.org/r/27531/#comment116546>

    indent args on next line.
    
    metrics.incrementTasksSourceReason(
        TASK_LOST,
        ...,
        ...);
        
        
    here and everywhere else.



src/master/master.cpp
<https://reviews.apache.org/r/27531/#comment116549>

    const &?



src/master/master.cpp
<https://reviews.apache.org/r/27531/#comment116551>

    Not sure why we missed updating "metrics" for non-terminal states when we 
introduced "metrics". This is a bug. I'll prep a fix for this.



src/master/metrics.hpp
<https://reviews.apache.org/r/27531/#comment116543>

    why is this named "tasks_source_reason" as opposed to 
"tasks_state_source_reason"? not that i like long names, but seems in line with 
what you were aiming for.
    
    anyway, for readability, how about:
    
    typedef hashmap<TaskStatus::Reason, process::metrics::Counter> Reasons;
    typedef hashmap<TaskStatus::Source, Reasons> SourcesReasons;
    
    hashmap<TaskState, SourcesReasons> tasks_states;



src/master/metrics.hpp
<https://reviews.apache.org/r/27531/#comment116536>

    Indent args on next line.
    
    void incrementTasksSourceReason(
        TaskState state,
        ...,
        ...);
    
    s/incrementTasksSourceReason/incrementTaskStates/?



src/master/metrics.cpp
<https://reviews.apache.org/r/27531/#comment116544>

    ditto. indentation.



src/slave/slave.cpp
<https://reviews.apache.org/r/27531/#comment116553>

    kill this.



src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/27531/#comment116555>

    It is a bit weird to see a test for these metrics in authorization tests. 
Any particular reason or did you randomly pick this file? I would recommend 
testing these in master_tests.cpp (and in slave_tests.cpp once you implement 
the slave metrics) since that already has metrics tests.



src/tests/master_authorization_tests.cpp
<https://reviews.apache.org/r/27531/#comment116557>

    ditto. see above.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/27531/#comment116558>

    s/add reason test/Add expectations for task source reason metrics./



src/tests/mesos.hpp
<https://reviews.apache.org/r/27531/#comment116560>

    Sweet. While you are at it, can you update rate_limiting_tests.cpp to use 
this instead? Also, kill METRICS_SNAPSHOT macro. would recommend doing this 
part as its own review.



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/27531/#comment116559>

    ditto. rephrase comment as suggested above.


- Vinod Kone


On Jan. 20, 2015, 9:26 p.m., Dominic Hamon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27531/
> -----------------------------------------------------------
> 
> (Updated Jan. 20, 2015, 9:26 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1830
>     https://issues.apache.org/jira/browse/MESOS-1830
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Update metrics in Master to match the source and reason split for task 
> statuses.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp b5fa8d19f031892922e4507f51280c83aa3e4a1a 
>   src/master/metrics.hpp 5e6b6d55ce03727f25140379c15d08bd5d70df38 
>   src/master/metrics.cpp a7ac96d34bbd84b5df74e1434624340d55b8651a 
>   src/slave/slave.hpp 29bea655a4ba8c99b3a81108b9224b8476927761 
>   src/slave/slave.cpp 06b2e18ff9b202c30f8bf4378cdd35aef734337f 
>   src/tests/master_authorization_tests.cpp 
> 796fe21c7936aaeef86a38984dcc6e455e92784b 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 04806ede4e727fa4de1464017a06797d69b54e29 
>   src/tests/master_tests.cpp 66423a9dd1bc441ad7207830a87ddff220df2031 
>   src/tests/mesos.hpp c1d64a73ff2312a10d4e809072d219e60c28a76f 
>   src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a 
>   src/tests/slave_tests.cpp 18ff8fe499943d209549d9924558d6e84ce78fec 
> 
> Diff: https://reviews.apache.org/r/27531/diff/
> 
> 
> Testing
> -------
> 
> added metric tests to master tests
> make check
> 
> 
> Thanks,
> 
> Dominic Hamon
> 
>