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)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jlal, Assigned: mshal)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1118008
So we definitely want the pushlog id, not just the buildid (date of the build)?
I am sure of the correct behavior around retriggers? We should decide what we want first.
From an IRC discussion, we decided on using the pushdate.
Assignee: nobody → mshal
Attached patch bug1150191-rank (obsolete) — Splinter Review
This grabs the pushdate from hg.m.o and uses it as the rank for the task.
Attachment #8588587 - Flags: review?(jlund)
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+
(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.
Attached patch bug1150191-rankSplinter Review
Updated with a try/except.
Attachment #8588587 - Attachment is obsolete: true
Attachment #8590460 - Flags: review?(jlund)
> 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 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+
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.

Attachment

General

Created:
Updated:
Size: