Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

2016-07-16 Thread Tuan-Anh Hoang-Vu 


> On July 16, 2016, 4:11 a.m., Abhishek Dasgupta wrote:
> > src/master/http.cpp, line 1947
> > 
> >
> > We can't put UNREACHABLE() here.May be we have to write here:
> > return Failure("Some Message");
> > 
> > Anyway, check it with your shepherd what you need to put here.

I see. Now I got it. UNREACHABLE() is to handle the `default` case, but we need 
to handle the `UNKNOWN` case differently.


- Tuan-Anh


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


On July 15, 2016, 11:13 a.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 15, 2016, 11:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

2016-07-16 Thread Abhishek Dasgupta 

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




src/master/http.cpp (line 1947)


We can't put UNREACHABLE() here.May be we have to write here:
return Failure("Some Message");

Anyway, check it with your shepherd what you need to put here.


- Abhishek Dasgupta


On July 15, 2016, 6:13 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 15, 2016, 6:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

2016-07-16 Thread Abhishek Dasgupta 

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




src/master/http.cpp (line 1948)


Move L1947 after this line


- Abhishek Dasgupta


On July 15, 2016, 6:13 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 15, 2016, 6:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

2016-07-15 Thread Tuan-Anh Hoang-Vu 

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

(Updated July 15, 2016, 11:13 a.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Updated Master::GET_METRICS call to return metrics grouped by types.


Diffs (updated)
-

  include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
  include/mesos/v1/master/master.proto c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
  src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
  src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 

Diff: https://reviews.apache.org/r/49938/diff/


Testing
---

Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
types.


1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
'{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
python -m json.tool


Thanks,

Tuan-Anh Hoang-Vu

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

2016-07-15 Thread Abhishek Dasgupta 

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




src/master/http.cpp (lines 1943 - 1945)


I just learned few days back that we don't use switch this way. We don't 
use `default` . Instead, we address UNKNOWN explicitly and use UNREACHABLE(). 
You may see :
Future Master::Http::listFiles(
const mesos::master::Call& call,
const Option& principal,
ContentType contentType) const
{
}
in the same file.


- Abhishek Dasgupta


On July 14, 2016, 9:35 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 14, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

2016-07-15 Thread Abhishek Dasgupta 


> On July 15, 2016, 6:55 a.m., Abhishek Dasgupta wrote:
> > src/master/http.cpp, lines 1917-1928
> > 
> >
> > You may do it this way:
> >  // TODO(tuananh): I don't know why I cannot use foreachpair instead.
> > foreachpair (const MetricType& metricType, const auto& metrics, 
> > metricsMap) {
> >   foreachpair (const string& key, double value, metrics) {
> > switch (metricType) {
> >   case MetricType::COUNTER:
> >   {
> > Metric metric;
> > metric.set_name(key);
> > metric.set_value(value);
> > _getMetrics->add_counters()->CopyFrom(metric);
> > break;
> >   }

Please, notice that I changed blocks inside switch cases a little. Can you do 
likewise for other blocks in this switch case if you address this comment of 
mine?


- Abhishek


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


On July 14, 2016, 9:35 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 14, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

2016-07-15 Thread Abhishek Dasgupta 

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




src/master/http.cpp (lines 1917 - 1928)


You may do it this way:
 // TODO(tuananh): I don't know why I cannot use foreachpair instead.
foreachpair (const MetricType& metricType, const auto& metrics, 
metricsMap) {
  foreachpair (const string& key, double value, metrics) {
switch (metricType) {
  case MetricType::COUNTER:
  {
Metric metric;
metric.set_name(key);
metric.set_value(value);
_getMetrics->add_counters()->CopyFrom(metric);
break;
  }


- Abhishek Dasgupta


On July 14, 2016, 9:35 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 14, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

2016-07-15 Thread Abhishek Dasgupta 

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




src/tests/api_tests.cpp (line 335)


Can you be specific here? eg., // Verifies that the response for `COUNTER` 
type metrics is not empty. Likewise, in the following ASSERT_LE tests.


- Abhishek Dasgupta


On July 14, 2016, 9:35 p.m., Tuan-Anh Hoang-Vu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49938/
> ---
> 
> (Updated July 14, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-5731
> https://issues.apache.org/jira/browse/MESOS-5731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Master::GET_METRICS call to return metrics grouped by types.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
>   include/mesos/v1/master/master.proto 
> c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
>   src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
>   src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 
> 
> Diff: https://reviews.apache.org/r/49938/diff/
> 
> 
> Testing
> ---
> 
> Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
> types.
> 
> 
> 1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
> '{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
> python -m json.tool
> 
> 
> Thanks,
> 
> Tuan-Anh Hoang-Vu
> 
>

Re: Review Request 49938: Updated Master::GET_METRICS call to return metrics grouped by types.

2016-07-14 Thread Tuan-Anh Hoang-Vu 

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

(Updated July 14, 2016, 2:35 p.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


Summary (updated)
-

Updated Master::GET_METRICS call to return metrics grouped by types.


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


Repository: mesos


Description (updated)
---

Updated Master::GET_METRICS call to return metrics grouped by types.


Diffs (updated)
-

  include/mesos/master/master.proto 7fb7153b5498a55469d2c72873c45fb534c0e60d 
  include/mesos/v1/master/master.proto c9e4359fff018de55d83da5cde0eb6d59dacfc3e 
  src/master/http.cpp 42ba3643daf8f6fe63152178214b57e3b773d93e 
  src/tests/api_tests.cpp 55e825ea6a3bd43c76dc67e8b90a97e8c9530a47 

Diff: https://reviews.apache.org/r/49938/diff/


Testing
---

Modified MasterAPITest.GetMetrics to make sure we return metrics grouped by 
types.


1. Run master: ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
2. Call GET_METRICS: curl --header "Content-Type:application/json" --data 
'{"type": "GET_METRICS", "get_metrics":{}}' http://localhost:5050/api/v1 | 
python -m json.tool


Thanks,

Tuan-Anh Hoang-Vu