Re: Review Request 66846: Added per framework metrics support for terminal task state.

2018-05-23 Thread Gilbert Song 


> On May 16, 2018, 10:34 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 12113 (patched)
> > 
> >
> > When `task->statuses.size == 0`, I think we probably want to increment 
> > the terminal task state metric using the contents of `task->state()`.

does `task->statuses.size == 0` case exist?


> On May 16, 2018, 10:34 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 12115 (patched)
> > 
> >
> > In this case, maybe we still want to increment the metric? We should 
> > probably figure out which of the two fields (`task->state()` or 
> > `task->statuses()`) should contain the more recent state and use that to 
> > increment the metric.
> > 
> > Or, do we expect this case to never happen?

I don't think this could happen. user warning instead of CHECK, just to be more 
safe


> On May 16, 2018, 10:34 a.m., Greg Mann wrote:
> > src/master/metrics.cpp
> > Lines 683 (patched)
> > 
> >
> > Is this temporary variable necessary? Here and below.

as mentioned in previous patches, it wouldn't compile due to unordered_map++ 
issue.


- Gilbert


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


On May 15, 2018, 5:45 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66846/
> ---
> 
> (Updated May 15, 2018, 5:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, Jie Yu, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-8912
> https://issues.apache.org/jira/browse/MESOS-8912
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added per framework metrics support for terminal task state.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 41862db9900acde85a62d2fea85459691c68556e 
>   src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
>   src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
> 
> 
> Diff: https://reviews.apache.org/r/66846/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66846: Added per framework metrics support for terminal task state.

2018-05-16 Thread Greg Mann 

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




src/master/master.cpp
Lines 12113 (patched)


When `task->statuses.size == 0`, I think we probably want to increment the 
terminal task state metric using the contents of `task->state()`.



src/master/master.cpp
Lines 12115 (patched)


In this case, maybe we still want to increment the metric? We should 
probably figure out which of the two fields (`task->state()` or 
`task->statuses()`) should contain the more recent state and use that to 
increment the metric.

Or, do we expect this case to never happen?



src/master/metrics.cpp
Lines 664-670 (patched)


Since these are default-constructed, I think you can just remove this block 
of code. Initialization will occur, if necessary, when you do
```
if (!terminal_task_reasons[state][source].contains(reason)) {
```



src/master/metrics.cpp
Lines 683 (patched)


Is this temporary variable necessary? Here and below.


- Greg Mann


On May 16, 2018, 12:45 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66846/
> ---
> 
> (Updated May 16, 2018, 12:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, Jie Yu, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-8912
> https://issues.apache.org/jira/browse/MESOS-8912
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added per framework metrics support for terminal task state.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 41862db9900acde85a62d2fea85459691c68556e 
>   src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
>   src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
> 
> 
> Diff: https://reviews.apache.org/r/66846/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66846: Added per framework metrics support for terminal task state.

2018-05-15 Thread Gilbert Song 

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

(Updated May 15, 2018, 5:45 p.m.)


Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, Jie Yu, 
and Vinod Kone.


Bugs: MESOS-8912
https://issues.apache.org/jira/browse/MESOS-8912


Repository: mesos


Description
---

Added per framework metrics support for terminal task state.


Diffs (updated)
-

  src/master/master.cpp 41862db9900acde85a62d2fea85459691c68556e 
  src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
  src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 


Diff: https://reviews.apache.org/r/66846/diff/5/

Changes: https://reviews.apache.org/r/66846/diff/4-5/


Testing
---


Thanks,

Gilbert Song

Re: Review Request 66846: Added per framework metrics support for terminal task state.

2018-05-11 Thread Gilbert Song 

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

(Updated May 11, 2018, 1:44 a.m.)


Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, Jie Yu, 
and Vinod Kone.


Repository: mesos


Description
---

Added per framework metrics support for terminal task state.


Diffs (updated)
-

  src/master/master.cpp 41862db9900acde85a62d2fea85459691c68556e 
  src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
  src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 


Diff: https://reviews.apache.org/r/66846/diff/4/

Changes: https://reviews.apache.org/r/66846/diff/3-4/


Testing
---


Thanks,

Gilbert Song

Re: Review Request 66846: Added per framework metrics support for terminal task state.

2018-05-11 Thread Gilbert Song 

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

(Updated May 11, 2018, 12:44 a.m.)


Review request for mesos, Benjamin Mahler, Gaston Kleiman, Greg Mann, Jie Yu, 
and Vinod Kone.


Summary (updated)
-

Added per framework metrics support for terminal task state.


Repository: mesos


Description (updated)
---

Added per framework metrics support for terminal task state.


Diffs (updated)
-

  src/master/master.cpp 41862db9900acde85a62d2fea85459691c68556e 
  src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
  src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 


Diff: https://reviews.apache.org/r/66846/diff/3/

Changes: https://reviews.apache.org/r/66846/diff/2-3/


Testing
---


Thanks,

Gilbert Song