Issue 2226583003: Add build scheduler DB interface, dumb in-memory impl, cache

borenet

borenet@google.com changed reviewers: + benjaminwagner@google.com, borenet@google.com

4 years, 4 months ago (2016-08-08 17:47:26 UTC) #1

dogben

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/cache.go File build_scheduler/go/db/cache.go (right): https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/cache.go#newcode17 build_scheduler/go/db/cache.go:17: // GetBuildsForCommits retrieves all builds which first included each ...

4 years, 4 months ago (2016-08-08 19:22:39 UTC) #3

borenet

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/cache.go File build_scheduler/go/db/cache.go (right): https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/cache.go#newcode17 build_scheduler/go/db/cache.go:17: // GetBuildsForCommits retrieves all builds which first included each ...

4 years, 4 months ago (2016-08-08 19:51:18 UTC) #4

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
File build_scheduler/go/db/cache.go (right):

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache.go:17: // GetBuildsForCommits retrieves all builds
which first included each of the
On 2016/08/08 19:22:38, Ben Wagner wrote:
> There are a couple things that I don't understand about the documentation for
> this method:
>  - Shouldn't/Doesn't this return the most recent build rather than the first
> build? Actually, I think BuildCache is agnostic as to time and revision
history
> -- it just finds builds where the blamelist includes the commits passed in.
>  - If we run two builds for the same builder at the same commit, is the client
> of BuildCache required to set Commits to the empty list for one of those
builds?
> If not, what does this method return in that case?

Historically, newer builds "steal" from the blamelist of previous ones.  In
other words, if a newer build runs at the same commit as a previous build, the
previous build's blamelist becomes empty, and the new build gets all the
commits. That's currently handled by datahopper as it ingests builds, but in the
new world it'll be handled by the build scheduler.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache.go:29: rv[commit][k] = v.Copy()
On 2016/08/08 19:22:38, Ben Wagner wrote:
> Method doc says the sub-map key is the builder name (which I think makes
sense),
> but in update() below (and in tests), the sub-map key is the build ID.

Fixed.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache.go:45: if old, ok := c.builds[b.Id]; ok {
On 2016/08/08 19:22:38, Ben Wagner wrote:
> We should think about whether we want to use BuildBucket's ID or some other
ID.
> 
> If we have a way to generate IDs for "fake" builds that won't conflict with
> BuildBucket IDs, then it's not really a concern.

Agreed. I'm just using Buildbucket's IDs for now for simplicity.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache.go:59: c.buildsByCommit[commit][b.Id] = b.Copy()
On 2016/08/08 19:22:38, Ben Wagner wrote:
> We should make a single copy and store it in both maps.

Done.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache.go:67: newBuilds, err :=
c.db.GetModifiedBuilds(c.queryId)
On 2016/08/08 19:22:38, Ben Wagner wrote:
> nit:
> 
> now := time.Now()
> newBuilds, err := c.db.GetModifiedBuilds(c.queryId)
> ...
> if err := c.update(newBuilds); err != nil {
>   c.lastUpdate = now
> }

Good catch, done.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache.go:112: bc.update(builds)
On 2016/08/08 19:22:38, Ben Wagner wrote:
> nit: check error (or change update to not return error)

Done.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
File build_scheduler/go/db/cache_test.go (right):

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache_test.go:11: func testGetBuildsForCommits(t
*testing.T, c *BuildCache, b *Build) {
On 2016/08/08 19:22:38, Ben Wagner wrote:
> Is there any reason to test GetBuildsForCommits for multiple input commits or
> multiple builds at the same commit?

If ingestion is done correctly, we should never have two builds for the same
builder with the same commit in the blamelist.  I don't know if it makes sense
to test that here yet, although I guess I could add some kind of check at
insertion time to enforce that?  Seems slow though.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/d...
File build_scheduler/go/db/db_test.go (right):

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/d...
build_scheduler/go/db/db_test.go:50: b1Before := b1Created.Add(-1 *
time.Millisecond)
On 2016/08/08 19:22:38, Ben Wagner wrote:
> nit: Can we make b1Before := b1Created, to test that GetBuildsFromDateRange
> treats the time range as inclusive begin, exclusive end?
> 
> Same for b2Before and b3Before.

Done, and fixed inMemoryDB to behave that way.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/m...
File build_scheduler/go/db/memory.go (right):

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/m...
build_scheduler/go/db/memory.go:58: rv = append(rv, b)
On 2016/08/08 19:22:38, Ben Wagner wrote:
> nit: Probably want to copy.
> 
> (Or change the docs on the interface to say the caller can't modify the
returned
> builds, but that seems error-prone.)

Done.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/m...
build_scheduler/go/db/memory.go:75: rv = append(rv, b)
On 2016/08/08 19:22:39, Ben Wagner wrote:
> nit: Probably want to copy.

Done.

dogben

lgtm https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/cache.go File build_scheduler/go/db/cache.go (right): https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/cache.go#newcode17 build_scheduler/go/db/cache.go:17: // GetBuildsForCommits retrieves all builds which first included ...

4 years, 4 months ago (2016-08-08 20:16:07 UTC) #5

lgtm

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
File build_scheduler/go/db/cache.go (right):

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache.go:17: // GetBuildsForCommits retrieves all builds
which first included each of the
On 2016/08/08 19:51:17, borenet wrote:
> On 2016/08/08 19:22:38, Ben Wagner wrote:
> > There are a couple things that I don't understand about the documentation
for
> > this method:
> >  - Shouldn't/Doesn't this return the most recent build rather than the first
> > build? Actually, I think BuildCache is agnostic as to time and revision
> history
> > -- it just finds builds where the blamelist includes the commits passed in.
> >  - If we run two builds for the same builder at the same commit, is the
client
> > of BuildCache required to set Commits to the empty list for one of those
> builds?
> > If not, what does this method return in that case?
> 
> Historically, newer builds "steal" from the blamelist of previous ones.  In
> other words, if a newer build runs at the same commit as a previous build, the
> previous build's blamelist becomes empty, and the new build gets all the
> commits. That's currently handled by datahopper as it ingests builds, but in
the
> new world it'll be handled by the build scheduler.

Is it worthwhile to mention here that we assume the blamelists are
non-overlapping?

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
File build_scheduler/go/db/cache_test.go (right):

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache_test.go:11: func testGetBuildsForCommits(t
*testing.T, c *BuildCache, b *Build) {
On 2016/08/08 19:51:18, borenet wrote:
> On 2016/08/08 19:22:38, Ben Wagner wrote:
> > Is there any reason to test GetBuildsForCommits for multiple input commits
or
> > multiple builds at the same commit?
> 
> If ingestion is done correctly, we should never have two builds for the same
> builder with the same commit in the blamelist.  I don't know if it makes sense
> to test that here yet, although I guess I could add some kind of check at
> insertion time to enforce that?  Seems slow though.

Sorry, I meant two builds for the same commit but different builders.

borenet

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/cache.go File build_scheduler/go/db/cache.go (right): https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/cache.go#newcode17 build_scheduler/go/db/cache.go:17: // GetBuildsForCommits retrieves all builds which first included each ...

4 years, 4 months ago (2016-08-09 11:22:55 UTC) #6

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
File build_scheduler/go/db/cache.go (right):

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache.go:17: // GetBuildsForCommits retrieves all builds
which first included each of the
On 2016/08/08 20:16:07, Ben Wagner wrote:
> On 2016/08/08 19:51:17, borenet wrote:
> > On 2016/08/08 19:22:38, Ben Wagner wrote:
> > > There are a couple things that I don't understand about the documentation
> for
> > > this method:
> > >  - Shouldn't/Doesn't this return the most recent build rather than the
first
> > > build? Actually, I think BuildCache is agnostic as to time and revision
> > history
> > > -- it just finds builds where the blamelist includes the commits passed
in.
> > >  - If we run two builds for the same builder at the same commit, is the
> client
> > > of BuildCache required to set Commits to the empty list for one of those
> > builds?
> > > If not, what does this method return in that case?
> > 
> > Historically, newer builds "steal" from the blamelist of previous ones.  In
> > other words, if a newer build runs at the same commit as a previous build,
the
> > previous build's blamelist becomes empty, and the new build gets all the
> > commits. That's currently handled by datahopper as it ingests builds, but in
> the
> > new world it'll be handled by the build scheduler.
> 
> Is it worthwhile to mention here that we assume the blamelists are
> non-overlapping?

Wrote some more doc, hopefully it's clearer now.

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
File build_scheduler/go/db/cache_test.go (right):

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache_test.go:11: func testGetBuildsForCommits(t
*testing.T, c *BuildCache, b *Build) {
On 2016/08/08 20:16:07, Ben Wagner wrote:
> On 2016/08/08 19:51:18, borenet wrote:
> > On 2016/08/08 19:22:38, Ben Wagner wrote:
> > > Is there any reason to test GetBuildsForCommits for multiple input commits
> or
> > > multiple builds at the same commit?
> > 
> > If ingestion is done correctly, we should never have two builds for the same
> > builder with the same commit in the blamelist.  I don't know if it makes
sense
> > to test that here yet, although I guess I could add some kind of check at
> > insertion time to enforce that?  Seems slow though.
> 
> Sorry, I meant two builds for the same commit but different builders.

Oh! Done.

borenet

The patchset sent to the CQ was uploaded after l-g-t-m from benjaminwagner@google.com Link to the ...

4 years, 4 months ago (2016-08-09 11:37:00 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2226583003/60001

4 years, 4 months ago (2016-08-09 11:37:07 UTC) #9

commit-bot: I haz the power

Description was changed from ========== Add build scheduler DB interface, dumb in-memory impl, cache BUG=skia: ...

4 years, 4 months ago (2016-08-09 11:42:44 UTC) #10

commit-bot: I haz the power

Committed patchset #4 (id:60001) as https://skia.googlesource.com/buildbot/+/341012f3e2766c49194fc9427fb519341c75dbbc

4 years, 4 months ago (2016-08-09 11:42:46 UTC) #11

dogben

4 years, 4 months ago (2016-08-09 13:56:06 UTC) #12

Message was sent while issue was closed.

lgtm

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
File build_scheduler/go/db/cache.go (right):

https://codereview.chromium.org/2226583003/diff/20001/build_scheduler/go/db/c...
build_scheduler/go/db/cache.go:17: // GetBuildsForCommits retrieves all builds
which first included each of the
On 2016/08/09 11:22:55, borenet wrote:
> On 2016/08/08 20:16:07, Ben Wagner wrote:
> > On 2016/08/08 19:51:17, borenet wrote:
> > > On 2016/08/08 19:22:38, Ben Wagner wrote:
> > > > There are a couple things that I don't understand about the
documentation
> > for
> > > > this method:
> > > >  - Shouldn't/Doesn't this return the most recent build rather than the
> first
> > > > build? Actually, I think BuildCache is agnostic as to time and revision
> > > history
> > > > -- it just finds builds where the blamelist includes the commits passed
> in.
> > > >  - If we run two builds for the same builder at the same commit, is the
> > client
> > > > of BuildCache required to set Commits to the empty list for one of those
> > > builds?
> > > > If not, what does this method return in that case?
> > > 
> > > Historically, newer builds "steal" from the blamelist of previous ones. 
In
> > > other words, if a newer build runs at the same commit as a previous build,
> the
> > > previous build's blamelist becomes empty, and the new build gets all the
> > > commits. That's currently handled by datahopper as it ingests builds, but
in
> > the
> > > new world it'll be handled by the build scheduler.
> > 
> > Is it worthwhile to mention here that we assume the blamelists are
> > non-overlapping?
> 
> Wrote some more doc, hopefully it's clearer now.

Awesome! Thanks!

Issue 2226583003: Add build scheduler DB interface, dumb in-memory impl, cache (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix import #

Patch Set 3 : Address some comments #

Patch Set 4 : Address more comments #

Messages