Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2438433004:
git cl try-results: add --wait-till-finished option. (Closed)
|
Created:
4 years, 2 months ago by tandrii(chromium) Modified:
3 years, 4 months ago CC:
chromium-reviews, tandrii+omg_git_cl_chromium.org, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptiongit cl try-results: add -w|--wait-till-finished option.
If -w|--wait-till-finished is specified, git cl try-results
will keep checking buildbucket for builds' status until
either:
* 1 hour passes
* all builds are completed (regardless of success or failure).
R=machenbach@chromium.org,sergiyb@chromium.org
Patch Set 1 #Patch Set 2 : git cl try-results: add --wait-till-finished option. #Patch Set 3 : git cl try-results: add --wait-till-finished option. #Patch Set 4 : Tests. #
Total comments: 9
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Depot Tools Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/31f5b1fdfe2a1710)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== git cl try-results: add --wait-till-finished option. If --wait-till-finished is specified, git cl try-results will keep BUG= ========== to ========== git cl try-results: add --wait-till-finished option. If --wait-till-finished is specified, git cl try-results will keep R=machenbach@chromium.org,sergiyb@chromium.org ==========
tandrii@chromium.org changed reviewers: + machenbach@chromium.org, sergiyb@chromium.org
PTAL This is a personal need of myself, bcz I prefer to keep staring at my terminal than refreshing Rietveld page. Still, maybe it should be not be landed at all! Feel free to say so.
Description was changed from ========== git cl try-results: add --wait-till-finished option. If --wait-till-finished is specified, git cl try-results will keep R=machenbach@chromium.org,sergiyb@chromium.org ========== to ========== git cl try-results: add -w|--wait-till-finished option. If -w|--wait-till-finished is specified, git cl try-results will keep checking buildbucket for builds' status until either: * 1 hour passes * all builds are completed (regardless of success or failure). R=machenbach@chromium.org,sergiyb@chromium.org ==========
lgtm > This is a personal need of myself, bcz I prefer to keep staring at my terminal > than refreshing Rietveld page. > > Still, maybe it should be not be landed at all! Feel free to say so. I think the feature is nice, but this kinda reminds me that you once said that git-cl has too many one-user features. How about getting a buy-in from developers on chromium-dev? Ask them to +1 if they intend to use it. If you get 10+ replies, then go for it, otherwise I'd say, don't land this. https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... git_cl.py:469: return sum(b['status'] != 'COMPLETED' for b in jobs.itervalues()) perhaps len(b for b in jobs.itervalues() if b['status'] != 'COMPLETED') is better than hiding implicit bool-to-int conversion here. https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... git_cl.py:4961: return 0 Please "break", so that if someone chooses to add something later in this func, then it will be executed https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... git_cl.py:4966: return 0 ditto https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... git_cl.py:4968: print('After %.0f seconds waiting, %d jobs still running\n\n' % Juse use %d instead of %.0f. It will round it to the next it... it's not the same because %.0f will take floor operation, but AFAIK this doesn't matter here and %d is simpler to understand. https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... git_cl.py:4973: time_sleep(10) # Limit unnecessary load on buildbucket. If the option becomes widely popular, we may have a lot of developers running this in parallel. A 1000 concurrently running commands with this option will cause 100 requests per second. But OTH, IMHO paying for 0.1 requests per second per developer is good value for the money, so this is probably reasonable. We can probably revisit this later if it does indeed become that popular.
https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... git_cl.py:4918: help='Keep checking buildbucket until either all jobs finish. ' nit: remove "either"? https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... git_cl.py:4957: write_try_results_json(options.json, jobs) Does this need to write the json in the loop? How about just once in the end? Also: This prints everything in a loop, maybe only the remaining ones should be printed? Or maybe use carriage return to keep modifying the same lines? https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... git_cl.py:4970: if passed > 3600: Do you need a limit at all? Is this tool supposed to be usable by automated infrastructure? If it's just intended for users maybe stay without limit and be robust to using Ctrl-C?
https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py File git_cl.py (right): https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... git_cl.py:4970: if passed > 3600: On 2016/10/20 09:40:35, machenbach (slow) wrote: > Do you need a limit at all? Is this tool supposed to be usable by automated > infrastructure? If it's just intended for users maybe stay without limit and be > robust to using Ctrl-C? IMHO, this is useful to prevent users forgetting that they are running their command in some screen shell and having it run for days causing unneeded load on buildbucket.
On 2016/10/20 11:44:43, Sergiy Byelozyorov wrote: > https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py > File git_cl.py (right): > > https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... > git_cl.py:4970: if passed > 3600: > On 2016/10/20 09:40:35, machenbach (slow) wrote: > > Do you need a limit at all? Is this tool supposed to be usable by automated > > infrastructure? If it's just intended for users maybe stay without limit and > be > > robust to using Ctrl-C? > > IMHO, this is useful to prevent users forgetting that they are running their > command in some screen shell and having it run for days causing unneeded load on > buildbucket. But that can happen only if their jobs never finish. They'd all eventually run into the buildbucket timeout and get cancelled.
On 2016/10/20 11:51:50, machenbach (slow) wrote: > On 2016/10/20 11:44:43, Sergiy Byelozyorov wrote: > > https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py > > File git_cl.py (right): > > > > > https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... > > git_cl.py:4970: if passed > 3600: > > On 2016/10/20 09:40:35, machenbach (slow) wrote: > > > Do you need a limit at all? Is this tool supposed to be usable by automated > > > infrastructure? If it's just intended for users maybe stay without limit and > > be > > > robust to using Ctrl-C? > > > > IMHO, this is useful to prevent users forgetting that they are running their > > command in some screen shell and having it run for days causing unneeded load > on > > buildbucket. > > But that can happen only if their jobs never finish. They'd all eventually run > into the buildbucket timeout and get cancelled. Anyway - was just a suggestion - have no strong opinion here.
On 2016/10/20 11:52:08, machenbach (slow) wrote: > On 2016/10/20 11:51:50, machenbach (slow) wrote: > > On 2016/10/20 11:44:43, Sergiy Byelozyorov wrote: > > > https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py > > > File git_cl.py (right): > > > > > > > > > https://chromiumcodereview.appspot.com/2438433004/diff/60001/git_cl.py#newcod... > > > git_cl.py:4970: if passed > 3600: > > > On 2016/10/20 09:40:35, machenbach (slow) wrote: > > > > Do you need a limit at all? Is this tool supposed to be usable by > automated > > > > infrastructure? If it's just intended for users maybe stay without limit > and > > > be > > > > robust to using Ctrl-C? > > > > > > IMHO, this is useful to prevent users forgetting that they are running their > > > command in some screen shell and having it run for days causing unneeded > load > > on > > > buildbucket. > > > > But that can happen only if their jobs never finish. They'd all eventually run > > into the buildbucket timeout and get cancelled. > > Anyway - was just a suggestion - have no strong opinion here. Indeed. I didn't think about that. Then I'd leave it up to Andrii. Timeout is a good safeguard, but not strongly needed here.
Message was sent while issue was closed.
abandoned in favor of Gerrit re-upload: https://chromium-review.googlesource.com/620771 |
