Closed
Bug 1150191
Opened 9 years ago
Closed 9 years ago
taskcluster indexing should be passed pushlog id as rank
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jlal, Assigned: mshal)
References
Details
Attachments
(1 file, 1 obsolete file)
6.75 KB,
patch
|
jlund
:
review+
mshal
:
checked-in+
|
Details | Diff | Splinter Review |
http://docs.taskcluster.net/services/index/ defines a "rank" (set via task.extra.index) which allows you to control how index overrides a namespace. This is primarily useful in the "latest" case which you want to mark a build as the "latest" on a particular branch. The pushlog id is useful here because it gives you an ordered number that relates to when you push. This means you can ensure the "latest" namespace will always be something newer then the last "latest" regardless of the order builds complete in.
Assignee | ||
Comment 1•9 years ago
|
||
So we definitely want the pushlog id, not just the buildid (date of the build)?
Reporter | ||
Comment 2•9 years ago
|
||
I am sure of the correct behavior around retriggers? We should decide what we want first.
Assignee | ||
Comment 3•9 years ago
|
||
From an IRC discussion, we decided on using the pushdate.
Assignee: nobody → mshal
Assignee | ||
Comment 4•9 years ago
|
||
This grabs the pushdate from hg.m.o and uses it as the rank for the task.
Attachment #8588587 -
Flags: review?(jlund)
Comment 5•9 years ago
|
||
Comment on attachment 8588587 [details] [diff] [review] bug1150191-rank Review of attachment 8588587 [details] [diff] [review]: ----------------------------------------------------------------- looks good. Have you tested it somewhere? sheep it. ::: mozharness/mozilla/building/buildbase.py @@ +28,5 @@ > import re > from mozharness.base.config import BaseConfig, parse_config_file > from mozharness.base.log import ERROR, OutputParser, FATAL, WARNING > from mozharness.base.script import PostScriptRun > +from mozharness.base.transfer import TransferMixin ha, I don't think I've come across this mixin yet. @@ +692,5 @@ > > self.buildid = buildid > return self.buildid > > + def query_pushdate(self): nice! non-blocking: this feels generic enough that maybe it could be in VCSMixin (http://mxr.mozilla.org/build/source/mozharness/mozharness/base/vcs/vcsbase.py#34) ? @@ +716,5 @@ > + # } > + # > + # So we grab the first element ("28537" in this case) and then pull out > + # the 'date' field. > + self.pushdate = contents.itervalues().next()['date'] seems fragile. do we need a catch or else clause? @@ +719,5 @@ > + # the 'date' field. > + self.pushdate = contents.itervalues().next()['date'] > + self.info('Pushdate is: %s' % self.pushdate) > + > + return self.pushdate so every build for this related push will share the extra.index correct?
Attachment #8588587 -
Flags: review?(jlund) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Jordan Lund (:jlund) from comment #5) > looks good. Have you tested it somewhere? sheep it. Yep, this try build (same as bug 1146892) has it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=531250a4e6c1 > @@ +692,5 @@ > > > > self.buildid = buildid > > return self.buildid > > > > + def query_pushdate(self): > > nice! > > non-blocking: this feels generic enough that maybe it could be in VCSMixin > (http://mxr.mozilla.org/build/source/mozharness/mozharness/base/vcs/vcsbase. > py#34) ? It uses _query_repo() and query_revision() also from BuildScript() though - I'm guessing the leading underscore on _query_repo() means it should be private to that class? > > @@ +716,5 @@ > > + # } > > + # > > + # So we grab the first element ("28537" in this case) and then pull out > > + # the 'date' field. > > + self.pushdate = contents.itervalues().next()['date'] > > seems fragile. do we need a catch or else clause? Good idea. I'll add an exception handler with an error message in the next version of the patch (once try looks good...) > @@ +719,5 @@ > > + # the 'date' field. > > + self.pushdate = contents.itervalues().next()['date'] > > + self.info('Pushdate is: %s' % self.pushdate) > > + > > + return self.pushdate > > so every build for this related push will share the extra.index correct? Yes, I think this is what we want. All builds of a particular push will have a defined rank, so the index for various platforms should generally point to the same build.
Assignee | ||
Comment 7•9 years ago
|
||
Updated with a try/except.
Attachment #8588587 -
Attachment is obsolete: true
Attachment #8590460 -
Flags: review?(jlund)
Comment 8•9 years ago
|
||
> It uses _query_repo() and query_revision() also from BuildScript() though -
> I'm guessing the leading underscore on _query_repo() means it should be
> private to that class?
hmm, ya I suppose it make sense for those to eventually be ripped out into a sub vcs mixin class too. but that sounds like work that is not part of this bug so ignore me :)
Comment 9•9 years ago
|
||
Comment on attachment 8590460 [details] [diff] [review] bug1150191-rank Review of attachment 8590460 [details] [diff] [review]: ----------------------------------------------------------------- assuming only the try/catch is the only addition
Attachment #8590460 -
Flags: review?(jlund) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8590460 [details] [diff] [review] bug1150191-rank https://hg.mozilla.org/build/mozharness/rev/878065b67729
Attachment #8590460 -
Flags: checked-in+
Comment 11•9 years ago
|
||
mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/production
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•