Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2049583003:
Add resource locking in gclient (Closed)
|
Created:
4 years, 6 months ago by Ryan Tseng Modified:
4 years, 6 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionAdd resource locking in gclient
There are entries in the DEPS file where two folders uses the same
git URL (ie. freetype2). This doesn't work well with git caches because
each task will run on it's own and might try to clobber on top of each other.
This adds another field in a WorkItem which is a list of resources. When the
work queue is flushed, it has to make sure that none of a newly added workitem
has any resource conflicts.
BUG=618124
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/885e5b1ee2f72dc80668fe426b410fe6ffc44f1d
Patch Set 1 #
Total comments: 4
Patch Set 2 : Review #
Total comments: 1
Patch Set 3 : Review and fix #Messages
Total messages: 35 (16 generated)
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049583003/1
hinoka@google.com changed reviewers: + sergeyberezin@chromium.org
I think this will fix the issue we were seeing
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Depot Tools Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)
Looks mostly good - just a couple of comments. Thanks! https://codereview.chromium.org/2049583003/diff/1/gclient.py File gclient.py (right): https://codereview.chromium.org/2049583003/diff/1/gclient.py#newcode380 gclient.py:380: url_segments = url.split('@') I'm not sure I understand this part - what's the "schema" for the url here? https://codereview.chromium.org/2049583003/diff/1/gclient_utils.py File gclient_utils.py (right): https://codereview.chromium.org/2049583003/diff/1/gclient_utils.py#newcode877 gclient_utils.py:877: print 'Checking resource %s' % used_resource Is this a debugging print, or do you intend to keep it in production? Might be too verbose.
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049583003/20001
hinoka@chromium.org changed reviewers: + hinoka@chromium.org
https://codereview.chromium.org/2049583003/diff/1/gclient.py File gclient.py (right): https://codereview.chromium.org/2049583003/diff/1/gclient.py#newcode380 gclient.py:380: url_segments = url.split('@') On 2016/06/08 01:28:36, Sergey Berezin wrote: > I'm not sure I understand this part - what's the "schema" for the url here? Added comment. https://codereview.chromium.org/2049583003/diff/1/gclient_utils.py File gclient_utils.py (right): https://codereview.chromium.org/2049583003/diff/1/gclient_utils.py#newcode877 gclient_utils.py:877: print 'Checking resource %s' % used_resource On 2016/06/08 01:28:36, Sergey Berezin wrote: > Is this a debugging print, or do you intend to keep it in production? Might be > too verbose. Yeah that was a debugging print... I'll keep it as logging.debug()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Depot Tools Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)
LGTM + nit (and of course, fix the bug - see presubmit failure). Thanks! https://codereview.chromium.org/2049583003/diff/20001/gclient.py File gclient.py (right): https://codereview.chromium.org/2049583003/diff/20001/gclient.py#newcode383 gclient.py:383: if url_segments > 0: nit: str.split('@') always returns a non-empty array, so this branch is redundant. However, url is not a str here - see presubmit failure.
The CQ bit was checked by hinoka@google.com to run a CQ dry run
Review and fix
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049583003/40001
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 hinoka@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyberezin@chromium.org Link to the patchset: https://codereview.chromium.org/2049583003/#ps40001 (title: "Review and fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049583003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Depot Tools Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)
hinoka@google.com changed reviewers: + iannucci@chromium.org
hinoka@google.com changed reviewers: + agable@chromium.org
+Owners plz. For various reasons, required for Win10 GCE bots.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
hm. I thought we already handled this case somewhere ... lgtm if that's not the case, though. And neither of you two is a depot_tools owner? That seems odd.
Unfortunately after some digging i couldn't find any hints of this case being taken care of. There is a concept of "locking" in git_cache, but iirc the mechanism is to hard fail if the lock is present, which isn't an ideal behavior. Seems like the least hacky way to do this is to make sure two deps with resource conflicts dont' get scheduled to run together.
The CQ bit was checked by hinoka@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2049583003/40001
Message was sent while issue was closed.
Description was changed from ========== Add resource locking in gclient There are entries in the DEPS file where two folders uses the same git URL (ie. freetype2). This doesn't work well with git caches because each task will run on it's own and might try to clobber on top of each other. This adds another field in a WorkItem which is a list of resources. When the work queue is flushed, it has to make sure that none of a newly added workitem has any resource conflicts. BUG=618124 ========== to ========== Add resource locking in gclient There are entries in the DEPS file where two folders uses the same git URL (ie. freetype2). This doesn't work well with git caches because each task will run on it's own and might try to clobber on top of each other. This adds another field in a WorkItem which is a list of resources. When the work queue is flushed, it has to make sure that none of a newly added workitem has any resource conflicts. BUG=618124 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/885e5b1ee2f72d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/885e5b1ee2f72d... |
