You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2017/08/19 00:27:14 UTC
Review Request 61766: Made the output handling of TCP and HTTP checks
consistent.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61766/
-----------------------------------------------------------
Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
Bugs: MESOS-7861
https://issues.apache.org/jira/browse/MESOS-7861
Repository: mesos
Description
-------
Made the output handling of TCP and HTTP checks consistent.
Diffs
-----
src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
Diff: https://reviews.apache.org/r/61766/diff/1/
Testing
-------
`make tests` on GNU/Linux
Thanks,
Gastón Kleiman
Re: Review Request 61766: Made the output handling of TCP and HTTP
checks consistent.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61766/#review183408
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made the output handling of TCP and HTTP checks consistent.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61766/diff/1/
>
>
> Testing
> -------
>
> `make tests` on GNU/Linux
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61766: Made the output handling of TCP and HTTP
checks consistent.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61766/#review183285
-----------------------------------------------------------
Bad patch!
Reviews applied: [61766, 61697]
Failed command: python support/apply-reviews.py -n -r 61697
Error:
Traceback (most recent call last):
File "support/apply-reviews.py", line 417, in <module>
main()
File "support/apply-reviews.py", line 412, in main
reviewboard(options)
File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
File "support/apply-reviews.py", line 160, in apply_review
commit_patch(options)
File "support/apply-reviews.py", line 268, in commit_patch
shell(cmd, options['dry_run'])
File "support/apply-reviews.py", line 144, in shell
error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
File "C:\Python27\lib\subprocess.py", line 168, in call
return Popen(*popenargs, **kwargs).wait()
File "C:\Python27\lib\subprocess.py", line 390, in __init__
errread, errwrite)
File "C:\Python27\lib\subprocess.py", line 610, in _execute_child
args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in position 25: ordinal not in range(128)
Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/256/console
- Mesos Reviewbot Windows
On Aug. 18, 2017, 5:27 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 5:27 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made the output handling of TCP and HTTP checks consistent.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61766/diff/1/
>
>
> Testing
> -------
>
> `make tests` on GNU/Linux
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61766: Made the output handling of TCP and HTTP
checks consistent.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 21, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 913-920 (original), 913-920 (patched)
> > <https://reviews.apache.org/r/61766/diff/1/?file=1800336#file1800336line913>
> >
> > Let's print stderr regardless of the retcode, what you now do with the command checks. It is probably not much value to do so here, but it is _consistent_ : ).
Thinking more about it, I lean towards your solution and not print anything unless there was a curl failure. HTTP checks are actually not meant to be processes, so printing `curl`'s stdout and stderr is not necessary.
> On Aug. 21, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 1071-1076 (original), 1075-1080 (patched)
> > <https://reviews.apache.org/r/61766/diff/1/?file=1800336#file1800336line1075>
> >
> > Let's print it regardless of the retcode, what you now do with the command checks. It is probably not much value to do so for the tcp checker, but it is _consistent_ : ).
Thinking more about it, I lean towards your solution and not print anything unless there was a `tcp-connect` failure. TCP checks are actually not meant to be processes, so printing `tcp-connect`'s stdout and stderr is not necessary.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61766/#review183332
-----------------------------------------------------------
On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made the output handling of TCP and HTTP checks consistent.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61766/diff/1/
>
>
> Testing
> -------
>
> `make tests` on GNU/Linux
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61766: Made the output handling of TCP and HTTP
checks consistent.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 21, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 916-919 (original), 916-919 (patched)
> > <https://reviews.apache.org/r/61766/diff/1/?file=1800336#file1800336line916>
> >
> > Looking at this, I'm not sure we should return a failure here: `stderr` is not critical for getting the HTTP code, so maybe log warning here? This is also consistent with what you now do for nested command checks.
In the nested command check case, the logging is performed in a void function, so returning a failure is not an option. I think that returning a failure here makes sense; while stderr isn't strictly necessary to get the status code, if we hit this case something has definitely gone wrong. The failure message will be logged by the executor and will be easily accessible for debugging.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61766/#review183332
-----------------------------------------------------------
On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made the output handling of TCP and HTTP checks consistent.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61766/diff/1/
>
>
> Testing
> -------
>
> `make tests` on GNU/Linux
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61766: Made the output handling of TCP and HTTP
checks consistent.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61766/#review183332
-----------------------------------------------------------
src/checks/checker_process.cpp
Lines 913-920 (original), 913-920 (patched)
<https://reviews.apache.org/r/61766/#comment259370>
Let's print stderr regardless of the retcode, what you now do with the command checks. It is probably not much value to do so here, but it is _consistent_ : ).
src/checks/checker_process.cpp
Lines 916-919 (original), 916-919 (patched)
<https://reviews.apache.org/r/61766/#comment259371>
Looking at this, I'm not sure we should return a failure here: `stderr` is not critical for getting the HTTP code, so maybe log warning here? This is also consistent with what you now do for nested command checks.
src/checks/checker_process.cpp
Lines 1071-1076 (original), 1075-1080 (patched)
<https://reviews.apache.org/r/61766/#comment259369>
Let's print it regardless of the retcode, what you now do with the command checks. It is probably not much value to do so for the tcp checker, but it is _consistent_ : ).
- Alexander Rukletsov
On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made the output handling of TCP and HTTP checks consistent.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61766/diff/1/
>
>
> Testing
> -------
>
> `make tests` on GNU/Linux
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61766: Made the output handling of TCP and HTTP
checks consistent.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 21, 2017, 1:46 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 921-924 (original), 921-924 (patched)
> > <https://reviews.apache.org/r/61766/diff/1/?file=1800336#file1800336line921>
> >
> > Additionally `VLOG(1)` stderr?
This failure message will be printed by the executor and it includes the stderr output, so I think this is sufficient?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61766/#review183336
-----------------------------------------------------------
On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made the output handling of TCP and HTTP checks consistent.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61766/diff/1/
>
>
> Testing
> -------
>
> `make tests` on GNU/Linux
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61766: Made the output handling of TCP and HTTP
checks consistent.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61766/#review183336
-----------------------------------------------------------
src/checks/checker_process.cpp
Lines 921-924 (original), 921-924 (patched)
<https://reviews.apache.org/r/61766/#comment259375>
Additionally `VLOG(1)` stderr?
- Alexander Rukletsov
On Aug. 19, 2017, 12:27 a.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61766/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2017, 12:27 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made the output handling of TCP and HTTP checks consistent.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61766/diff/1/
>
>
> Testing
> -------
>
> `make tests` on GNU/Linux
>
>
> Thanks,
>
> Gastón Kleiman
>
>