Re: Review Request 17255: Add observe endpoint to master.

2014-03-20 Thread Charlie Carson 


> On March 20, 2014, 6:45 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 551
> > 
> >
> > Could you follow up with a fix for this? It should be bound to 
> > Http::stats, not roles.

opened https://issues.apache.org/jira/browse/MESOS-1129 to track this since the 
review is closed and code is in


- Charlie


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


On Feb. 13, 2014, 6:55 p.m., Charlie Carson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17255/
> ---
> 
> (Updated Feb. 13, 2014, 6:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jeff Currier.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add observe endpoint to master.
> 
> This changes adds a new HTTP endpoint of observe to master.
> This allows clients to report health via an HTTP POST.
> Values are:
>   MONITOR = Monitor for which health is being reported.
>   HOSTS = Comma seperated list of hosts.
>   LEVEL = OK for healthy, anything else for unhealthy.
> 
> This also contains a small fix to alphabetize the existing
> endpoints / help strings.
> 
> SEE:  https://issues.apache.org/jira/browse/MESOS-880
> 
> Review: https://reviews.apache.org/r/17255
> 
> 
> Diffs
> -
> 
>   src/Makefile.am aa8bb2b492e4d37b14db294f48c074e631143b68 
>   src/master/http.cpp 966eed6d8340038265ef799f1b6149502ccc606e 
>   src/master/master.hpp 737bd8b5a1cd88a98a7f094795c50547079921ba 
>   src/master/master.cpp a4e1b1f7327f01eb16f14c3fa08fc7a62048d36c 
>   src/tests/repair_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/17255/diff/
> 
> 
> Testing
> ---
> 
> make check
> added unit tests in a new repair_test.cpp file
> 
> 
> Thanks,
> 
> Charlie Carson
> 
>

Re: Review Request 17255: Add observe endpoint to master.

2014-03-20 Thread Dominic Hamon 

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



src/master/master.cpp


Could you follow up with a fix for this? It should be bound to Http::stats, 
not roles.


- Dominic Hamon


On Feb. 13, 2014, 10:55 a.m., Charlie Carson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17255/
> ---
> 
> (Updated Feb. 13, 2014, 10:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jeff Currier.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add observe endpoint to master.
> 
> This changes adds a new HTTP endpoint of observe to master.
> This allows clients to report health via an HTTP POST.
> Values are:
>   MONITOR = Monitor for which health is being reported.
>   HOSTS = Comma seperated list of hosts.
>   LEVEL = OK for healthy, anything else for unhealthy.
> 
> This also contains a small fix to alphabetize the existing
> endpoints / help strings.
> 
> SEE:  https://issues.apache.org/jira/browse/MESOS-880
> 
> Review: https://reviews.apache.org/r/17255
> 
> 
> Diffs
> -
> 
>   src/Makefile.am aa8bb2b492e4d37b14db294f48c074e631143b68 
>   src/master/http.cpp 966eed6d8340038265ef799f1b6149502ccc606e 
>   src/master/master.hpp 737bd8b5a1cd88a98a7f094795c50547079921ba 
>   src/master/master.cpp a4e1b1f7327f01eb16f14c3fa08fc7a62048d36c 
>   src/tests/repair_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/17255/diff/
> 
> 
> Testing
> ---
> 
> make check
> added unit tests in a new repair_test.cpp file
> 
> 
> Thanks,
> 
> Charlie Carson
> 
>

Re: Review Request 17255: Add observe endpoint to master.

2014-02-13 Thread Charlie Carson 

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

(Updated Feb. 13, 2014, 6:55 p.m.)


Review request for mesos, Benjamin Hindman and Jeff Currier.


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

https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880


Repository: mesos-git


Description
---

Add observe endpoint to master.

This changes adds a new HTTP endpoint of observe to master.
This allows clients to report health via an HTTP POST.
Values are:
  MONITOR = Monitor for which health is being reported.
  HOSTS = Comma seperated list of hosts.
  LEVEL = OK for healthy, anything else for unhealthy.

This also contains a small fix to alphabetize the existing
endpoints / help strings.

SEE:  https://issues.apache.org/jira/browse/MESOS-880

Review: https://reviews.apache.org/r/17255


Diffs (updated)
-

  src/Makefile.am aa8bb2b492e4d37b14db294f48c074e631143b68 
  src/master/http.cpp 966eed6d8340038265ef799f1b6149502ccc606e 
  src/master/master.hpp 737bd8b5a1cd88a98a7f094795c50547079921ba 
  src/master/master.cpp a4e1b1f7327f01eb16f14c3fa08fc7a62048d36c 
  src/tests/repair_tests.cpp PRE-CREATION 

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


Testing
---

make check
added unit tests in a new repair_test.cpp file


Thanks,

Charlie Carson

Re: Review Request 17255: Add observe endpoint to master.

2014-02-10 Thread Vinod Kone 

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

Ship it!


Nice test.


src/master/http.cpp


We always use braces for if/for/while loops.

if () {

}



src/master/http.cpp


ditto.



src/master/http.cpp


ditto.



src/master/http.cpp


s/either/of/ ?



src/tests/repair_tests.cpp


s/r/response/



src/tests/repair_tests.cpp


s/o/object/



src/tests/repair_tests.cpp


How about using a ternary operator for conciseness?



src/tests/repair_tests.cpp


s/!ok/level != OK/ ?


- Vinod Kone


On Feb. 11, 2014, 12:10 a.m., Charlie Carson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17255/
> ---
> 
> (Updated Feb. 11, 2014, 12:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jeff Currier.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add observe endpoint to master.
> 
> This changes adds a new HTTP endpoint of observe to master.
> This allows clients to report health via an HTTP POST.
> Values are:
>   MONITOR = Monitor for which health is being reported.
>   HOSTS = Comma seperated list of hosts.
>   LEVEL = OK for healthy, anything else for unhealthy.
> 
> This also contains a small fix to alphabetize the existing
> endpoints / help strings.
> 
> SEE:  https://issues.apache.org/jira/browse/MESOS-880
> 
> Review: https://reviews.apache.org/r/17255
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
>   src/master/http.cpp fb15483953a593bfec4e60884219dc8c4e8d565c 
>   src/master/master.hpp 7649737283f5f5d786ac40504e943a3be4c1d62b 
>   src/master/master.cpp 241b0069902cd125fdd933096ddd7e1c841d1559 
>   src/tests/repair_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/17255/diff/
> 
> 
> Testing
> ---
> 
> make check
> added unit tests in a new repair_test.cpp file
> 
> 
> Thanks,
> 
> Charlie Carson
> 
>

Re: Review Request 17255: Add observe endpoint to master.

2014-02-10 Thread Charlie Carson 

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

(Updated Feb. 11, 2014, 12:10 a.m.)


Review request for mesos, Benjamin Hindman and Jeff Currier.


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

https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880


Repository: mesos-git


Description
---

Add observe endpoint to master.

This changes adds a new HTTP endpoint of observe to master.
This allows clients to report health via an HTTP POST.
Values are:
  MONITOR = Monitor for which health is being reported.
  HOSTS = Comma seperated list of hosts.
  LEVEL = OK for healthy, anything else for unhealthy.

This also contains a small fix to alphabetize the existing
endpoints / help strings.

SEE:  https://issues.apache.org/jira/browse/MESOS-880

Review: https://reviews.apache.org/r/17255


Diffs (updated)
-

  src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
  src/master/http.cpp fb15483953a593bfec4e60884219dc8c4e8d565c 
  src/master/master.hpp 7649737283f5f5d786ac40504e943a3be4c1d62b 
  src/master/master.cpp 241b0069902cd125fdd933096ddd7e1c841d1559 
  src/tests/repair_tests.cpp PRE-CREATION 

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


Testing (updated)
---

make check
added unit tests in a new repair_test.cpp file


Thanks,

Charlie Carson

Re: Review Request 17255: Add observe endpoint to master.

2014-02-10 Thread Charlie Carson 


> On Feb. 10, 2014, 7:10 a.m., Benjamin Hindman wrote:
> > src/Makefile.am, line 848
> > 
> >
> > Maybe s/health_tests/observe_tests/ ... although I assume you want to 
> > add more tests beyond just the observe endpoint? Since we have a 'health' 
> > endpoint there is a little ambiguity here.

renamed to repair_tests since the rest of the tests will be on the 
RepairCoordinator


- Charlie


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


On Jan. 30, 2014, 10:13 p.m., Charlie Carson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17255/
> ---
> 
> (Updated Jan. 30, 2014, 10:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jeff Currier.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add observe endpoint to master.
> 
> This changes adds a new HTTP endpoint of observe to master.
> This allows clients to report health via an HTTP POST.
> Values are:
>   MONITOR = Monitor for which health is being reported.
>   HOSTS = Comma seperated list of hosts.
>   LEVEL = OK for healthy, anything else for unhealthy.
> 
> This also contains a small fix to alphabetize the existing
> endpoints / help strings.
> 
> SEE:  https://issues.apache.org/jira/browse/MESOS-880
> 
> Review: https://reviews.apache.org/r/17255
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
>   src/master/http.cpp fb15483953a593bfec4e60884219dc8c4e8d565c 
>   src/master/master.hpp 7649737283f5f5d786ac40504e943a3be4c1d62b 
>   src/master/master.cpp 77872ece66601043ddeb280cceba5a7676e8a6be 
>   src/tests/health_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/17255/diff/
> 
> 
> Testing
> ---
> 
> make check
> added unit tests in a new health_test.cpp file
> 
> 
> Thanks,
> 
> Charlie Carson
> 
>

Re: Review Request 17255: Add observe endpoint to master.

2014-02-09 Thread Benjamin Hindman 


> On Jan. 29, 2014, 3:26 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, line 363
> > 
> >
> > Do you want to check for 'jsonp'?
> 
> Charlie Carson wrote:
> I'm not opposed, but not 100% it's useful either.  jsonp is typically so 
> that you can invoke a local call back when the response comes back, right?  
> ex:  
> 
>  src="http://server2.example.com/Users/1234?jsonp=parseResponse";>
> 
> 
> would invoke parseResponse w/ the result of the get to the resource.
> 
> the reason i'm not sure it makes sense is that this upload is a post, 
> which you can't do with a script tag.
> 
> OTOH, I might be ignorant of some other valid use cases.  dropping for 
> now - happy to reopen :-)

Makes sense to ignore for a POST!


- Benjamin


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


On Jan. 30, 2014, 10:13 p.m., Charlie Carson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17255/
> ---
> 
> (Updated Jan. 30, 2014, 10:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jeff Currier.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add observe endpoint to master.
> 
> This changes adds a new HTTP endpoint of observe to master.
> This allows clients to report health via an HTTP POST.
> Values are:
>   MONITOR = Monitor for which health is being reported.
>   HOSTS = Comma seperated list of hosts.
>   LEVEL = OK for healthy, anything else for unhealthy.
> 
> This also contains a small fix to alphabetize the existing
> endpoints / help strings.
> 
> SEE:  https://issues.apache.org/jira/browse/MESOS-880
> 
> Review: https://reviews.apache.org/r/17255
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
>   src/master/http.cpp fb15483953a593bfec4e60884219dc8c4e8d565c 
>   src/master/master.hpp 7649737283f5f5d786ac40504e943a3be4c1d62b 
>   src/master/master.cpp 77872ece66601043ddeb280cceba5a7676e8a6be 
>   src/tests/health_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/17255/diff/
> 
> 
> Testing
> ---
> 
> make check
> added unit tests in a new health_test.cpp file
> 
> 
> Thanks,
> 
> Charlie Carson
> 
>

Re: Review Request 17255: Add observe endpoint to master.

2014-02-09 Thread Benjamin Hindman 

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

Ship it!



src/Makefile.am


Maybe s/health_tests/observe_tests/ ... although I assume you want to add 
more tests beyond just the observe endpoint? Since we have a 'health' endpoint 
there is a little ambiguity here.



src/tests/health_tests.cpp


{ on newline please.



src/tests/health_tests.cpp


{ on newline please. Also, const& on JsonResponse?



src/tests/health_tests.cpp


These macros look great!


- Benjamin Hindman


On Jan. 30, 2014, 10:13 p.m., Charlie Carson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17255/
> ---
> 
> (Updated Jan. 30, 2014, 10:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jeff Currier.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add observe endpoint to master.
> 
> This changes adds a new HTTP endpoint of observe to master.
> This allows clients to report health via an HTTP POST.
> Values are:
>   MONITOR = Monitor for which health is being reported.
>   HOSTS = Comma seperated list of hosts.
>   LEVEL = OK for healthy, anything else for unhealthy.
> 
> This also contains a small fix to alphabetize the existing
> endpoints / help strings.
> 
> SEE:  https://issues.apache.org/jira/browse/MESOS-880
> 
> Review: https://reviews.apache.org/r/17255
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
>   src/master/http.cpp fb15483953a593bfec4e60884219dc8c4e8d565c 
>   src/master/master.hpp 7649737283f5f5d786ac40504e943a3be4c1d62b 
>   src/master/master.cpp 77872ece66601043ddeb280cceba5a7676e8a6be 
>   src/tests/health_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/17255/diff/
> 
> 
> Testing
> ---
> 
> make check
> added unit tests in a new health_test.cpp file
> 
> 
> Thanks,
> 
> Charlie Carson
> 
>

Re: Review Request 17255: Add observe endpoint to master.

2014-01-30 Thread Charlie Carson 


> On Jan. 29, 2014, 3:26 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, line 297
> > 
> >
> > You guys have likely already discussed this, but 'monitor' or 'metric'?

1) snapping to koalabird, just so we don't need to change something there 
and/or create a translation layer
2) to me, monitor implies something like "Disk space is low" vs. metric implies 
"Available disk space".  I think we want the former, b/c that allows the 
evaluation to be distributed (either by the external alerting system or on the 
host itself) rather than force it to take place on the host.  That's especially 
good here, b/c a given host / service might have very different ideas of how 
much available disk space is enough.  I DEFINITELY don't think we want master 
having knowledge of different classes of clients and what constitutes "good" or 
"bad" for a given metric / host.


> On Jan. 29, 2014, 3:26 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, line 299
> > 
> >
> > Likewise, 'level' makes it sound like there will be a spectrum of 
> > health. Is that planned for the future? Will it be a spectrum or more like 
> > a collection? If so, does s/level/status/ make more sense? These 
> > "interface" questions are harder to change later. ;)

again, koalabird terminology, and we can get either OK, WARN, or CRITICAL.  

personally, I would definitely prefer normalized degrees of failure, for things 
like disk space or memory free.  In my perfect world this would be a 0-N metric 
where 0 is healthy and the bigger the number the more unhealthy something is.  
it would be up to the host / external system to map "2GB free" to 0, 1, or 2 in 
terms of health.  this would let you prioritize repairs without having to do 
the evaluation.  OTOH, for the type of repairs we are expecting to do (hardware 
failure basically), it is pretty likely that binary will suffice (Jeff's 
experience).

I guess I'm trying to balance 3 different things:
1) snap to koalabird since that's what we need
2) keep it open ended, i.e. even if we thought it was binary, I would take 0 or 
1, not true or false, so that we allow degrees in the future if need be
3) internally, be strongly typed, b/c we can change that going forward.

but agreed there is lots of room for debate here.


> On Jan. 29, 2014, 3:26 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, line 308
> > 
> >
> > Please wrap lines longer than 80 characters. In this case, please wrap 
> > after the '=', indent by 2 on the newline.

fixed and I added this to my .vimrc so that it won't keep happening:

augroup vimrc_autocmds
  autocmd BufEnter * highlight ExtraWhitespace ctermbg=red guibg=red
  autocmd BufEnter * call matchadd('ExtraWhitespace', '\s\+$', 11)

  autocmd BufEnter * highlight OverLength ctermbg=red guibg=red
  autocmd BufEnter * call matchadd('OverLength', '\%>80v.\+')
augroup END


> On Jan. 29, 2014, 3:26 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, line 350
> > 
> >
> > Did you need this explicit assignment?

no, I've been coding in Java for too long :-)


> On Jan. 29, 2014, 3:26 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, line 363
> > 
> >
> > Do you want to check for 'jsonp'?

I'm not opposed, but not 100% it's useful either.  jsonp is typically so that 
you can invoke a local call back when the response comes back, right?  ex:  

http://server2.example.com/Users/1234?jsonp=parseResponse";>


would invoke parseResponse w/ the result of the get to the resource.

the reason i'm not sure it makes sense is that this upload is a post, which you 
can't do with a script tag.

OTOH, I might be ignorant of some other valid use cases.  dropping for now - 
happy to reopen :-) 


> On Jan. 29, 2014, 3:26 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, line 361
> > 
> >
> > Yeah, a JSON::Boolean would be great!

there is another CR out to fix it :-)


> On Jan. 29, 2014, 3:26 a.m., Benjamin Hindman wrote:
> > src/master/http.cpp, line 365
> > 
> >
> > Do you want to keep this in post testing? Assuming you don't include 
> > this, I think you can take two tacks for dealing with missing values here: 
> > (1) show all missing values and (2) show a missing value when you encounter 
> > it. In the latter case I'd prefer to see code above which explicitly checks 
> > for each key and returns BadRequest("Missing '" + KEY + "'.") when it is 
> > missing something. In addition, the key/value might be present but the 
> > v

Re: Review Request 17255: Add observe endpoint to master.

2014-01-30 Thread Charlie Carson 

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

(Updated Jan. 30, 2014, 10:13 p.m.)


Review request for mesos, Benjamin Hindman and Jeff Currier.


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

https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880


Repository: mesos-git


Description
---

Add observe endpoint to master.

This changes adds a new HTTP endpoint of observe to master.
This allows clients to report health via an HTTP POST.
Values are:
  MONITOR = Monitor for which health is being reported.
  HOSTS = Comma seperated list of hosts.
  LEVEL = OK for healthy, anything else for unhealthy.

This also contains a small fix to alphabetize the existing
endpoints / help strings.

SEE:  https://issues.apache.org/jira/browse/MESOS-880

Review: https://reviews.apache.org/r/17255


Diffs (updated)
-

  src/Makefile.am c30706846bca1fa3287291e39f46a23713ad1ba4 
  src/master/http.cpp fb15483953a593bfec4e60884219dc8c4e8d565c 
  src/master/master.hpp 7649737283f5f5d786ac40504e943a3be4c1d62b 
  src/master/master.cpp 77872ece66601043ddeb280cceba5a7676e8a6be 
  src/tests/health_tests.cpp PRE-CREATION 

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


Testing (updated)
---

make check
added unit tests in a new health_test.cpp file


Thanks,

Charlie Carson

Re: Review Request 17255: Add observe endpoint to master.

2014-01-28 Thread Benjamin Hindman 

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


Stoked to see a HELP included from the very beginning! In addition to the 
comments I made below it would be great to see a simple test. We've got this 
great little utility in libprocess called 'http::post' which should help you. ;)


src/master/http.cpp


You guys have likely already discussed this, but 'monitor' or 'metric'?



src/master/http.cpp


Likewise, 'level' makes it sound like there will be a spectrum of health. 
Is that planned for the future? Will it be a spectrum or more like a 
collection? If so, does s/level/status/ make more sense? These "interface" 
questions are harder to change later. ;)



src/master/http.cpp


Please wrap lines longer than 80 characters. In this case, please wrap 
after the '=', indent by 2 on the newline.



src/master/http.cpp


s/string &/string& /g (here and elsewhere please)



src/master/http.cpp


s/http/HTTP/



src/master/http.cpp


s/if(/if (/



src/master/http.cpp


s/json/JSON/g (although, it looks like you capitalized it everywhere else)



src/master/http.cpp


FYI, I prefer going through each key explicitly down here versus the loop 
of keys above.



src/master/http.cpp


Did you need this explicit assignment?



src/master/http.cpp


What about grouping each 'response.values[KEY]' block? Something like:

JSON::Object response;

// Add 'monitor'.
response.values[MONITOR_KEY] = values[MONITOR_KEY];

// Add 'hosts'.
vector hosts = strings::split(values[HOSTS_KEY], ",");

JSON::Array array;
array.values.assign(hosts.begin(), hosts.end());

response.values[HOSTS_KEY] = array;

// Add 'level'.
bool isHealthy = strings::upper(values[LEVEL_KEY]) == "OK";

// TODO(ccarson): This is a workaround ...
response.values[LEVEL_KEY] =
  (isHealthy ? JSON::Value(JSON::True()) : JSON::False());

return OK(response ...





src/master/http.cpp


s/workaroudn/workaround/



src/master/http.cpp


Yeah, a JSON::Boolean would be great!



src/master/http.cpp


Do you want to check for 'jsonp'?



src/master/http.cpp


Do you want to keep this in post testing? Assuming you don't include this, 
I think you can take two tacks for dealing with missing values here: (1) show 
all missing values and (2) show a missing value when you encounter it. In the 
latter case I'd prefer to see code above which explicitly checks for each key 
and returns BadRequest("Missing '" + KEY + "'.") when it is missing something. 
In addition, the key/value might be present but the value isn't decodable 
(http::decode fails) or is empty, both of which could be presented immediately 
as errors. It's not clear to me that there is that much benefit in determining 
all the ways in which the endpoint was misused (i.e., all the errors) rather 
than just returning after you encounter the first error.


- Benjamin Hindman


On Jan. 23, 2014, 7:09 p.m., Charlie Carson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17255/
> ---
> 
> (Updated Jan. 23, 2014, 7:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jeff Currier.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-880
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add observe endpoint to master.
> 
> This changes adds a new HTTP endpoint of observe to master.
> This allows clients to report health via an HTTP POST.
> Values are:
>   MONITOR = Monitor for which health is being reported.
>   HOSTS = Comma seperated list of hosts.
>   LEVEL = OK for healthy, anything else for unhealthy.
> 
> This also contains a small fix to alphabetize the existing
> endpoints / help strings.
> 
> SEE:  https://issues.apache.org/jira/browse/MESOS-880
> 
> Review: http

Review Request 17255: Add observe endpoint to master.

2014-01-23 Thread Charlie Carson 

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

Review request for mesos, Benjamin Hindman and Jeff Currier.


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

https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-880


Repository: mesos-git


Description
---

Add observe endpoint to master.

This changes adds a new HTTP endpoint of observe to master.
This allows clients to report health via an HTTP POST.
Values are:
  MONITOR = Monitor for which health is being reported.
  HOSTS = Comma seperated list of hosts.
  LEVEL = OK for healthy, anything else for unhealthy.

This also contains a small fix to alphabetize the existing
endpoints / help strings.

SEE:  https://issues.apache.org/jira/browse/MESOS-880

Review: https://reviews.apache.org/r/17255


Diffs
-

  src/master/http.cpp 546e91dbb9c8ee1014bb4f0b3be2714ad6a2d520 
  src/master/master.hpp 99b81811d596a777ee83dce7b157c1b11c064fab 
  src/master/master.cpp c7d9186b9c22fb40b05aa6d5bc6f2d38fb7f73ea 

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


Testing
---

make check
manually testing - I'll add unit tests when the repair coordinator is 
introduced in the next checkin.


Thanks,

Charlie Carson