|
|
Created:
8 years, 9 months ago by Peter Mayo Modified:
8 years, 8 months ago CC:
chromium-reviews, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionIncrease timeout for X starting.
Increase logging of what we checked, and how long it took.
BUG=103882
TEST=Run locally.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130123
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Messages
Total messages: 18 (0 generated)
More tracking down why virtual X sometimes breaks. In my test platform it doesn't seem to need retries at all, but I'd like to see in a few more realistic situations.
LGTM http://codereview.chromium.org/9802038/diff/2002/tools/xdisplaycheck/xdisplay... File tools/xdisplaycheck/xdisplaycheck.cc (right): http://codereview.chromium.org/9802038/diff/2002/tools/xdisplaycheck/xdisplay... tools/xdisplaycheck/xdisplaycheck.cc:52: fprintf(stderr, "Connected after %d tries\n", tries); stdout (here and 76)? Or do those not get logged?
http://codereview.chromium.org/9802038/diff/2002/tools/xdisplaycheck/xdisplay... File tools/xdisplaycheck/xdisplaycheck.cc (right): http://codereview.chromium.org/9802038/diff/2002/tools/xdisplaycheck/xdisplay... tools/xdisplaycheck/xdisplaycheck.cc:52: fprintf(stderr, "Connected after %d tries\n", tries); On 2012/03/28 21:09:39, sadrul wrote: > stdout (here and 76)? Or do those not get logged? These are still tracing messages which go to stderr by convention.
http://codereview.chromium.org/9802038/diff/5001/tools/xdisplaycheck/xdisplay... File tools/xdisplaycheck/xdisplaycheck.cc (right): http://codereview.chromium.org/9802038/diff/5001/tools/xdisplaycheck/xdisplay... tools/xdisplaycheck/xdisplaycheck.cc:44: Sleep(300); Why sleeping more versus trying more often?
On 2012/03/28 22:31:48, Marc-Antoine Ruel wrote: > http://codereview.chromium.org/9802038/diff/5001/tools/xdisplaycheck/xdisplay... > File tools/xdisplaycheck/xdisplaycheck.cc (right): > > http://codereview.chromium.org/9802038/diff/5001/tools/xdisplaycheck/xdisplay... > tools/xdisplaycheck/xdisplaycheck.cc:44: Sleep(300); > Why sleeping more versus trying more often? Because trying more often could just make the loading slower. 50 still seems like a lot of tries.
On 2012/03/29 15:03:59, Peter Mayo wrote: > Because trying more often could just make the loading slower. 50 still seems > like a lot of tries. Yes but increasing it to 300ms means the best case is still a waste of 200ms on every test on every slave. 50 * 100 = 5 seconds + the delay open the port. It's rather slow but I'm fine to waiting 15 seconds or so, as you are doing. I'd simply retry more often, unless you tell me XOpenDisplay() is such a CPU-bound function that it will adversely affect the xvfb start up when called at 10hz.
On 2012/03/29 15:07:17, Marc-Antoine Ruel wrote: > On 2012/03/29 15:03:59, Peter Mayo wrote: > > Because trying more often could just make the loading slower. 50 still seems > > like a lot of tries. > > Yes but increasing it to 300ms means the best case is still a waste of 200ms on > every test on every slave. > > 50 * 100 = 5 seconds + the delay open the port. It's rather slow but I'm fine to > waiting 15 seconds or so, as you are doing. I'd simply retry more often, unless > you tell me XOpenDisplay() is such a CPU-bound function that it will adversely > affect the xvfb start up when called at 10hz. In all of my tests it catches the first one. (i.e. 0 retries) How about linear backoff?
lgtm in any case, sorry for the annoyance.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petermayo@chromium.org/9802038/8002
Try job failure for 9802038-8002 (retry) on win_rel for steps "ui_tests, browser_tests". It's a second try, previously, steps "ui_tests, browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petermayo@chromium.org/9802038/8002
Try job failure for 9802038-8002 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petermayo@chromium.org/9802038/8002
Try job failure for 9802038-8002 (retry) on win_rel for step "browser_tests". It's a second try, previously, steps "browser_tests, installer_util_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petermayo@chromium.org/9802038/8002
Follows on to http://codereview.chromium.org/10008101/ http://codereview.chromium.org/9802038/diff/17004/tools/xdisplaycheck/xdispla... File tools/xdisplaycheck/xdisplaycheck.cc (right): http://codereview.chromium.org/9802038/diff/17004/tools/xdisplaycheck/xdispla... tools/xdisplaycheck/xdisplaycheck.cc:6: // continually retries until it connects or 5 seconds pass. If it fails Should update the timeout in these comments too
http://codereview.chromium.org/9802038/diff/17004/tools/xdisplaycheck/xdispla... File tools/xdisplaycheck/xdisplaycheck.cc (right): http://codereview.chromium.org/9802038/diff/17004/tools/xdisplaycheck/xdispla... tools/xdisplaycheck/xdisplaycheck.cc:6: // continually retries until it connects or 5 seconds pass. If it fails On 2012/04/13 02:01:05, Peter Mayo wrote: > Should update the timeout in these comments too You are self-reviewing your CL? :)
Self-reviewing after submission no less. If you never review mistakes, how can you learn from them? On Fri, Apr 13, 2012 at 2:56 PM, <maruel@chromium.org> wrote: > > http://codereview.chromium.**org/9802038/diff/17004/tools/** > xdisplaycheck/xdisplaycheck.cc<http://codereview.chromium.org/9802038/diff/17004/tools/xdisplaycheck/xdisplaycheck.cc> > File tools/xdisplaycheck/**xdisplaycheck.cc (right): > > http://codereview.chromium.**org/9802038/diff/17004/tools/** > xdisplaycheck/xdisplaycheck.**cc#newcode6<http://codereview.chromium.org/9802038/diff/17004/tools/xdisplaycheck/xdisplaycheck.cc#newcode6> > tools/xdisplaycheck/**xdisplaycheck.cc:6: // continually retries until it > connects or 5 seconds pass. If it fails > On 2012/04/13 02:01:05, Peter Mayo wrote: > >> Should update the timeout in these comments too >> > > You are self-reviewing your CL? :) > > http://codereview.chromium.**org/9802038/<http://codereview.chromium.org/9802... > -- Peter Mayo | Waterloo | petermayo@google.com | 519-880-3439 |