Re: code review 35480043: dashboard/app: benchmarking support (data model + build... (issue 35480043)

[dvy...@google.com]

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handler.go
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handler.go#newcode117
dashboard/app/build/handler.go:117: cr, err := GetCommitRun(c, com.Num)
This does not work as of now -- it fails with "transaction touches
several entity groups", because can go to a subrepo, but CommitRuns are
currently always descendants of main Package.
We have 2 choices:
1. Do not update CommitRun's for commits that does not need benchmarking
(in particular go to subrepos)
2. Make CommitRun descendants on a proper Package

I think we need to do both.
There is no particular value in CommitRun for non-benchmark commits. And
we can always recreate them solely on server (no need for builder
interaction, all info is already on the server).

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handler.go#newcode337
dashboard/app/build/handler.go:337: t := datastore.NewQuery("Commit").
this needs to scan CommitRun's

https://codereview.appspot.com/35480043/

[dvy...@google.com]

PTAL

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handler.go
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handler.go#newcode117
dashboard/app/build/handler.go:117: cr, err := GetCommitRun(c, com.Num)

On 2013/12/03 07:25:35, dvyukov wrote:
> This does not work as of now -- it fails with "transaction touches
several
> entity groups", because can go to a subrepo, but CommitRuns are
currently always
> descendants of main Package.
> We have 2 choices:
> 1. Do not update CommitRun's for commits that does not need
benchmarking (in
> particular go to subrepos)
> 2. Make CommitRun descendants on a proper Package

> I think we need to do both.
> There is no particular value in CommitRun for non-benchmark commits.
And we can
> always recreate them solely on server (no need for builder
interaction, all info
> is already on the server).

Done

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handler.go#newcode337
dashboard/app/build/handler.go:337: t := datastore.NewQuery("Commit").

On 2013/12/03 07:25:35, dvyukov wrote:
> this needs to scan CommitRun's

Done.

https://codereview.appspot.com/35480043/

[a...@golang.org]

Looks pretty good


https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go
File dashboard/app/build/build.go (right):

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode100
dashboard/app/build/build.go:100: Benchmark bool // Needs benchmarking?
BenchmarkPending?

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode110
dashboard/app/build/build.go:110: PerfBenchmarks []string
`datastore:",noindex"`
s/PerfBenchmarks/PerfResults/

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode234
dashboard/app/build/build.go:234: func GetCommitRun(c appengine.Context,
CommitNum int) (*CommitRun, error) {
commitNum

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode272
dashboard/app/build/build.go:272: func GetCommits(c appengine.Context,
StartCommitNum, N int) ([]*Commit, error) {
startCommitNum, n int

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode355
dashboard/app/build/build.go:355: // Descendant of Package.
Shouldn't this be a descendant of Commit?

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode393
dashboard/app/build/build.go:393: func GetPerfMetricRun(c
appengine.Context, Builder, Benchmark, Metric string, CommitNum int)
(*PerfMetricRun, error) {
lowercase args please, here and elsewhere

https://codereview.appspot.com/35480043/

[a...@golang.org]

Can you add some tests to test.go?

https://codereview.appspot.com/35480043/

[dvy...@google.com]

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go
File dashboard/app/build/build.go (right):

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode100
dashboard/app/build/build.go:100: Benchmark bool // Needs benchmarking?

On 2013/12/04 03:10:40, adg wrote:
> BenchmarkPending?

It was irritating me as well.
It's not a temporal property of a commit, so I think BenchmarkPending is
a bad name. E.g. when a new benchmark is added, all commits need
benchmarking again. PerfTodo tracks pending benchmarks.
I've renamed it to NeedsBenchmarking.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode110
dashboard/app/build/build.go:110: PerfBenchmarks []string
`datastore:",noindex"`

On 2013/12/04 03:10:40, adg wrote:
> s/PerfBenchmarks/PerfResults/

Done.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode234
dashboard/app/build/build.go:234: func GetCommitRun(c appengine.Context,
CommitNum int) (*CommitRun, error) {

On 2013/12/04 03:10:40, adg wrote:
> commitNum

Done.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode272
dashboard/app/build/build.go:272: func GetCommits(c appengine.Context,
StartCommitNum, N int) ([]*Commit, error) {

On 2013/12/04 03:10:40, adg wrote:
> startCommitNum, n int

Done.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode355
dashboard/app/build/build.go:355: // Descendant of Package.

On 2013/12/04 03:10:40, adg wrote:
> Shouldn't this be a descendant of Commit?

Everything else here is descendant of Package, including Result which is
the same as PerfResult in this regard. I've done the same.
Maybe it does not matter lot on our scale.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go#newcode393
dashboard/app/build/build.go:393: func GetPerfMetricRun(c
appengine.Context, Builder, Benchmark, Metric string, CommitNum int)
(*PerfMetricRun, error) {

On 2013/12/04 03:10:40, adg wrote:
> lowercase args please, here and elsewhere

Done.

https://codereview.appspot.com/35480043/

[dvy...@google.com]

PTAL

https://codereview.appspot.com/35480043/

[dvy...@google.com]

On 2013/12/04 03:11:45, adg wrote:
> Can you add some tests to test.go?

Done

https://codereview.appspot.com/35480043/

[a...@golang.org]

LGTM


https://codereview.appspot.com/35480043/diff/180001/dashboard/app/build/test.go
File dashboard/app/build/test.go (right):

https://codereview.appspot.com/35480043/diff/180001/dashboard/app/build/test.go#newcode229
dashboard/app/build/test.go:229: defer func() {
what does this accomplish?

https://codereview.appspot.com/35480043/

[dvy...@google.com]

https://codereview.appspot.com/35480043/diff/180001/dashboard/app/build/test.go
File dashboard/app/build/test.go (right):

https://codereview.appspot.com/35480043/diff/180001/dashboard/app/build/test.go#newcode229
dashboard/app/build/test.go:229: defer func() {

On 2013/12/04 21:51:38, adg wrote:
> what does this accomplish?

w/o this it crashes in net/http guts when the handler finishes
actually I am not sure how kosher this is
but at least it works right now, as opposed to current code

https://codereview.appspot.com/35480043/

[Andrew Gerrand]

On 5 December 2013 13:09, <dvy...@google.com> wrote:

w/o this it crashes in net/http guts when the handler finishes
actually I am not sure how kosher this is
but at least it works right now, as opposed to current code

That's weird. Oh well. We can fix it and use appengine/aetest to make all this a bit easier.

Andrew

[dvy...@google.com]

*** Submitted as c137fbe89798 ***

dashboard/app: benchmarking support (data model + builder handlers)
Adds new fields PerfBenchmarks and Benchmark to Commit.
Adds new entities -- CommitRun, PerfResult, PerfMetricRun, PerfTodo and
PerfConfig.
Adds new "benchmark-go-commit" todo.
Adds perf-result handler.
The design doc:
https://docs.google.com/a/google.com/document/d/10KInCeTYmpk7UZZLDGNktsisjctQnfm7FfOtr8RbFks/pub

R=adg
CC=golang-dev
https://codereview.appspot.com/35480043


https://codereview.appspot.com/35480043/