Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
250983003:
Added TryJobGerritScheduler (Closed)
|
Created:
6 years, 8 months ago by nodir Modified:
6 years, 7 months ago CC:
chromium-reviews, kjellander-cc_chromium.org, cmp-cc_chromium.org, ilevy-cc_chromium.org, stip+watch_chromium.org Visibility:
Public. |
DescriptionAdded TryJobGerritScheduler
The TryJobGerritScheduler uses TryJobGerritPoller to poll open issues on
Gerrit with TryJob=1 label. For each issue it searches for a 'new' message in
that starts with !tryjob prefix. A 'new' message means its date is
greater than previous poll time.
If !tryjob is followed by json, it is parsed as a job description. The
format is {'builderNames': <list of strings>}. The short form for this
is just a list, e.g. ['Stable'].
For each try job, TryJobGerritScheduler/TryJobGerritPoller create
1) a new buildbot change with the Gerrit patchset revision
2) a Sourcestamp
3) a Buildset with specified builder names. If not specified, the
default builder list is used.
The TryJobGerritPoller is a part of TryJobGerritScheduler and not
designed to be used otherwise.
This was tested locally with the following CL as input:
https://quickoffice-internal-review.googlesource.com/#/c/11081/1
BUG=366099, 364639, 364631
R=kmg@chromium.org, vadimsh@chromium.org, dnj@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=266962
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #Patch Set 3 : added GerritPoller.change_category #
Total comments: 25
Patch Set 4 : renamed comment to message #Patch Set 5 : Addressed Vadim's comments #Patch Set 6 : 'except' simplification in JobDefinition.parse #
Total comments: 1
Patch Set 7 : nit #Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... File scripts/master/try_job_gerrit.py (right): https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:63: self.scheduler = scheduler I don't think the 'change source' / 'scheduler' set is supposed to be directly aware of each other. If the scheduler is so tightly coupled, consider just moving the scheduler's "SubmitJob" etc. functions into this class. https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:71: query += '+label:TryJob=%2B1' While you're constraining the query, you might as well also constrain by the projects in the "projects" array. https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:88: self.scheduler.submitJob(buildbotChange, job) This needs to be yielded, doesn't it? https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:93: class TryJobGerritScheduler(SingleBranchScheduler): It would be nice if you could use a less heavy-handed scheduler. I recommend setting the 'category' field in the 'addChange' to something like 'tryjob' and have a simple scheduler that filters on "category='tryjob'". That way, you don't have to implement all of this plumbing.
https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... File scripts/master/try_job_gerrit.py (right): https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:108: builderNames=default_builder_names, Are you somehow running PRESUBMIT.py for the jobs scheduled off of Gerrit? Being able to have PRESUBMIT.py generate a preset list of default builders and steps to run is pretty useful.
https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... File scripts/master/try_job_gerrit.py (right): https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:63: self.scheduler = scheduler In general, yes, a poller and scheduler don't know about each other. They talk to each other through buildbot changes, but in here TryJobGerritPoller is essentially a part of TryJobGerritScheduler because the scheduler needs the tryjob definition, not available in a change. I would like not to put it into properties. Anyway, the can't work without each other. Note that addBuildbotChange method is in the poller, while addBuildsetForXXX methods are in the scheduler, so they can't be completely moved to one of the classes. I decided to do this way because pollers usually create changes and schedulers usually create sourcestamps and buildsets. https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:71: query += '+label:TryJob=%2B1' On 2014/04/26 00:38:51, Dan Jacques wrote: > While you're constraining the query, you might as well also constrain by the > projects in the "projects" array. I think I will wait for your GerritAgent https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:88: self.scheduler.submitJob(buildbotChange, job) On 2014/04/26 00:38:51, Dan Jacques wrote: > This needs to be yielded, doesn't it? Done. https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:93: class TryJobGerritScheduler(SingleBranchScheduler): On 2014/04/26 00:38:51, Dan Jacques wrote: > It would be nice if you could use a less heavy-handed scheduler. I recommend > setting the 'category' field in the 'addChange' to something like 'tryjob' and > have a simple scheduler that filters on "category='tryjob'". That way, you don't > have to implement all of this plumbing. This scheduler takes the builder names from the job description. Standard schedulers do have a mechanism to dynamically specify the list of builders. Still, I inherited TryJobGerritScheduler from BaseScheduler (the simplest one), added a category to a buildbot change and used addBuildsetForChanges, so the source stamp is created by the BaseScheduler. https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:108: builderNames=default_builder_names, This code lives on server. The git-try will run PRESUBMIT and post a job definition in a Gerrit comment, which will be parsed by this code
On 2014/04/28 22:31:10, nodir wrote: > https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... > File scripts/master/try_job_gerrit.py (right): > > https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... > scripts/master/try_job_gerrit.py:63: self.scheduler = scheduler > In general, yes, a poller and scheduler don't know about each other. They talk > to each other through buildbot changes, but in here TryJobGerritPoller is > essentially a part of TryJobGerritScheduler because the scheduler needs the > tryjob definition, not available in a change. I would like not to put it into > properties. Anyway, the can't work without each other. > > Note that addBuildbotChange method is in the poller, while addBuildsetForXXX > methods are in the scheduler, so they can't be completely moved to one of the > classes. I decided to do this way because pollers usually create changes and > schedulers usually create sourcestamps and buildsets. > > https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... > scripts/master/try_job_gerrit.py:71: query += '+label:TryJob=%2B1' > On 2014/04/26 00:38:51, Dan Jacques wrote: > > While you're constraining the query, you might as well also constrain by the > > projects in the "projects" array. > > I think I will wait for your GerritAgent > > https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... > scripts/master/try_job_gerrit.py:88: self.scheduler.submitJob(buildbotChange, > job) > On 2014/04/26 00:38:51, Dan Jacques wrote: > > This needs to be yielded, doesn't it? > > Done. > > https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... > scripts/master/try_job_gerrit.py:93: class > TryJobGerritScheduler(SingleBranchScheduler): > On 2014/04/26 00:38:51, Dan Jacques wrote: > > It would be nice if you could use a less heavy-handed scheduler. I recommend > > setting the 'category' field in the 'addChange' to something like 'tryjob' and > > have a simple scheduler that filters on "category='tryjob'". That way, you > don't > > have to implement all of this plumbing. > > This scheduler takes the builder names from the job description. Standard > schedulers do have a mechanism to dynamically specify the list of builders. > > Still, I inherited TryJobGerritScheduler from BaseScheduler (the simplest one), > added a category to a buildbot change and used addBuildsetForChanges, so the > source stamp is created by the BaseScheduler. > > https://codereview.chromium.org/250983003/diff/40001/scripts/master/try_job_g... > scripts/master/try_job_gerrit.py:108: builderNames=default_builder_names, > This code lives on server. The git-try will run PRESUBMIT and post a job > definition in a Gerrit comment, which will be parsed by this code Looks good to me, but you should get someone else to review it also.
lgtm
The CQ bit was checked by nodir@chromium.org
Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nodir@chromium.org/250983003/80001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
(FYI: I'm reviewing this now and I'm 'super star committer').
Expecting a super review..
On 2014/04/29 00:51:09, nodir wrote: > Expecting a super review.. It's not related to OWNERS, but is it related to the gold stars all over some of the cubicle walls around you guys?
https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... File scripts/master/gerrit_poller.py (right): https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... scripts/master/gerrit_poller.py:45: path = '/changes/?q=%s&n=1' % self.getChangeQuery() So if try server was offline and brought back online, it would skip all try jobs scheduled when it was offline? https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... scripts/master/gerrit_poller.py:61: def _is_interesting_comment(self, comment): # pylint: disable=R0201 Let's follow Gerrit REST API and call that entity a message. Gerrit calls it ChangeMessageInfo: https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#ch... And then consistently pass it as whole dict everywhere. https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... scripts/master/gerrit_poller.py:113: def addChange(self, (change, comment)): I'd prefer we not use structured args. That's unusual feature. i.e. make 'addChange(self, change, comment)' and in lambda below 'addChange(x[0], x[1])'. https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... scripts/master/gerrit_poller.py:116: def processChanges(self, j, since): Somehow I can't shake a feeling this code has subtle race conditions :( https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... File scripts/master/try_job_gerrit.py (right): https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:16: class JobDefinition(object): Consider using collections.namedtuple + factory function instead. It would give you immutable class + good __repr__ for free. https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:21: if b] 'if b' can probably fit on previous line also wrap (builder_name or []) in () to better readability. https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:32: return JobDefinition() Is it valuable? What happens if it is scheduled? https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:40: job = {'builderNames': job} elif not isinstance(job, dict): raise ValueError() (or something like that). https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:54: MESSAGE_REGEX_TRYJOB = re.compile('Patch set \d+:\s+\!tryjob', re.I) Each message contains 'Patch set XXX:' as a header? Hm... https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:62: def _is_interesting_comment(self, comment): # pylint: disable=R0201 Do you still need this pylint disable here? https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:76: job_def_str = msg[tryjob_match.end():] Use tryjob_match.group(<index>) here instead of .end():. You would need to add (.*) to regexp. https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:88: log.err('TryJobGerritPoller failed: %s' % e) Do you need to reraise an exception? Won't caller be stuck waiting for results (probably not..). Did you test this code path? Also Exception is too generic... Do you know what exactly is caught by this block? If so, I'd prefer more close-to-source except blocks. For example, json.loads(...) can totally be wrapped in 'except ValueError' to handle json deserialization errors.
On 2014/04/29 01:17:26, Kevin Graney wrote: > On 2014/04/29 00:51:09, nodir wrote: > > Expecting a super review.. > > It's not related to OWNERS, but is it related to the gold stars all over some of > the cubicle walls around you guys? I think you have to be in Chromium committers list to be 'super start committer' :) @chromium.org account and read-only SVN access is not enough. Linked page (http://www.chromium.org/getting-involved/become-a-committer) explains it.
Good review, thanks https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... File scripts/master/gerrit_poller.py (right): https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... scripts/master/gerrit_poller.py:45: path = '/changes/?q=%s&n=1' % self.getChangeQuery() On 2014/04/29 01:22:45, Vadim Sh. wrote: > So if try server was offline and brought back online, it would skip all try jobs > scheduled when it was offline? I've intentionally didn't improve this part much because of the upcoming enhanced GerritAgent https://codereview.chromium.org/246363002/ https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... scripts/master/gerrit_poller.py:61: def _is_interesting_comment(self, comment): # pylint: disable=R0201 On 2014/04/29 01:22:45, Vadim Sh. wrote: > Let's follow Gerrit REST API and call that entity a message. Gerrit calls it > ChangeMessageInfo: > https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#ch... > > And then consistently pass it as whole dict everywhere. Done. https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... scripts/master/gerrit_poller.py:113: def addChange(self, (change, comment)): On 2014/04/29 01:22:45, Vadim Sh. wrote: > I'd prefer we not use structured args. That's unusual feature. i.e. make > 'addChange(self, change, comment)' and in lambda below 'addChange(x[0], x[1])'. Done https://codereview.chromium.org/250983003/diff/80001/scripts/master/gerrit_po... scripts/master/gerrit_poller.py:116: def processChanges(self, j, since): On 2014/04/29 01:22:45, Vadim Sh. wrote: > Somehow I can't shake a feeling this code has subtle race conditions :( This will be also change with the new GerritAgent https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... File scripts/master/try_job_gerrit.py (right): https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:16: class JobDefinition(object): On 2014/04/29 01:22:45, Vadim Sh. wrote: > Consider using collections.namedtuple + factory function instead. It would give > you immutable class + good __repr__ for free. Decided not to change this because I prefer 1) the parse method in the class 2) validation at the construction level 3) factory function to be a type (possible by inheriting from named tuple with __new__, but complex) https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:21: if b] On 2014/04/29 01:22:45, Vadim Sh. wrote: > 'if b' can probably fit on previous line > also wrap (builder_name or []) in () to better readability. I am not sure it fits the rules https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... It says, "Each portion must fit on one line: mapping expression, for clause, filter expression." https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:32: return JobDefinition() On 2014/04/29 01:22:45, Vadim Sh. wrote: > Is it valuable? What happens if it is scheduled? Yes. The default list of builders is used https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:40: job = {'builderNames': job} On 2014/04/29 01:22:45, Vadim Sh. wrote: > elif not isinstance(job, dict): > raise ValueError() > > (or something like that). Done. https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:54: MESSAGE_REGEX_TRYJOB = re.compile('Patch set \d+:\s+\!tryjob', re.I) On 2014/04/29 01:22:45, Vadim Sh. wrote: > Each message contains 'Patch set XXX:' as a header? Hm... It does https://quickoffice-internal-review.googlesource.com/#/c/11081/1 https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:62: def _is_interesting_comment(self, comment): # pylint: disable=R0201 On 2014/04/29 01:22:45, Vadim Sh. wrote: > Do you still need this pylint disable here? Done. https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:76: job_def_str = msg[tryjob_match.end():] On 2014/04/29 01:22:45, Vadim Sh. wrote: > Use tryjob_match.group(<index>) here instead of .end():. You would need to add > (.*) to regexp. Done. https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:88: log.err('TryJobGerritPoller failed: %s' % e) On 2014/04/29 01:22:45, Vadim Sh. wrote: > Do you need to reraise an exception? Won't caller be stuck waiting for results > (probably not..). Did you test this code path? added raise and tested. This prints a stack trace to the log and does NOT crash the server. > Also Exception is too generic... Do you know what exactly is caught by this > block? If so, I'd prefer more close-to-source except blocks. For example, > json.loads(...) can totally be wrapped in 'except ValueError' to handle json > deserialization errors. I don't know what exception to expect. Since I've added 'raise', it's probably fine now. Now I raise ValueError if json.loads failed, see above
https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... File scripts/master/try_job_gerrit.py (right): https://codereview.chromium.org/250983003/diff/80001/scripts/master/try_job_g... scripts/master/try_job_gerrit.py:21: if b] On 2014/04/29 04:01:00, nodir wrote: > On 2014/04/29 01:22:45, Vadim Sh. wrote: > > 'if b' can probably fit on previous line > > also wrap (builder_name or []) in () to better readability. > > I am not sure it fits the rules > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... > > It says, "Each portion must fit on one line: mapping expression, for clause, > filter expression." Well, I can tell you we are not following this particular rule in tools/build/* code base. It's better to be consistent with the existing code, than with the rule no one follows... self.builder_names = [str(b) for b in (builder_names or []) if b] https://codereview.chromium.org/250983003/diff/140001/scripts/master/try_job_... File scripts/master/try_job_gerrit.py (right): https://codereview.chromium.org/250983003/diff/140001/scripts/master/try_job_... scripts/master/try_job_gerrit.py:81: return JobDefinition.parse(job_def_str) nit: no need for job_def_str variable, just return JobDefinition.parse(tryjob_match.group(1))
nit address, please approve
On 2014/04/29 20:26:29, nodir wrote: > nit address, please approve s/address/addressed
lgtm
The CQ bit was checked by nodir@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nodir@chromium.org/250983003/160001
Message was sent while issue was closed.
Change committed as 266962 |
