code review 154080043: dashboard/app: ignore, try not to create partial Commits

dashboard/app: ignore, try not to create partial Commits

It looks like the partial Commits are coming from the build breakages mails.
If you have commit A newer than commit B, then there are two code
paths depending on which reports its build result first.
For slow development, B finishes before A is committed, so when
A notices a failure it checks to see if B was okay.
That code path seems to be okay.
For submit of back-to-back changes, typically A finishes before B,
so when B notices an okay it checks to see if A failed.
That code path seems to zero the Commit for A while
trying to set its FailNotificationSent to true.
It does (did) succeed in setting FailNotificationSent to true,
it just zeroed everything else.

This CL adds code to refuse to do the datastore.Put to
update FailNotificationSent if the Commit info is incomplete.

It also tries to ignore Num=0 records, but that may not be
as important anymore.

[cmang, 2014-10-07 18:08:46]

LGTM

[rsc, 2014-10-07 19:28:42]

Hello cmang@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.tools

[cmang, 2014-10-07 19:35:35]

On 2014/10/07 19:28:42, rsc wrote:
> Hello mailto:cmang@golang.org (cc:
mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go.tools

LGTM

[rsc, 2014-10-07 19:37:12]

And there it is:

2014-10-07 12:08:00.165 refusing to notify with com={PackagePath:
Hash:c7857de341434044e406f03ca4c29dbcb66e55df ParentHash: Num:0 User: Desc:
Time:0001-01-01 00:00:00 +0000 UTC NeedsBenchmarking:false TryPatch:false
ResultData:[] PerfResults:[] FailNotificationSent:false}
goroutine 357 [running]:
build.commonNotify(0x7f39ecfb42f8, 0xc0109afd20, 0xc010a15c60,
0xc010ae4be0, 0x12, ...)
build/notify.go:222 +0x3ed
build.sendPerfFailMail(0x7f39ecfb42f8, 0xc0109afd20, 0xc010ae4be0, 0x12,
0xc010866dc0, ...)
build/notify.go:216 +0x334
build.checkPerfChanges(0x7f39ecfb42f8, 0xc0109afd20, 0xc010be9000,
0xc01093fe70, 0xc010ae4be0, ...)
build/handler.go:706 +0x5e2
build.addPerfResult(0x7f39ecfb42f8, 0xc0109afd20, 0xc010be9000,
0xc011563620, 0x177ef30, ...)
build/handler.go:640 +0x753
build.func·005(0x7f39ecfb42f8, 0xc0109afd20, 0xc, 0x177ef30)
build/handler.go:606 +0x5e
appengine/datastore.runOnce(0x7f39ecfb3668, 0xc010c26f00, 0x7f39ece23590,
0x0, 0x0, ...)
go/src/pkg/appengine/datastore/transaction.go:74 +0x2dc
appengine/datastore.RunInTransaction(0x7f39ecfb3668, 0xc010c26f00,
0x7f39ece23590, 0x0, 0xc010bc1480, ...)
go/src/pkg/appengine/datastore/transaction.go:119 +0x14e
build.perfResultHandler(0xc010be9000, 0x0, 0x0, 0x0, 0x0)
build/handler.go:608 +0x5a5
build.func·006(0x7f39ecfb3630, 0xc010c26f00, 0xc010be9000)
build/handler.go:815 +0x322
net/http.HandlerFunc.ServeHTTP(0xc010968710, 0x7f39ecfb3630, 0xc010c26f00,
0xc010be9000)
go/src/pkg/net/http/server.go:1220 +0x55
net/http.(*ServeMux).ServeHTTP(0xc0108554e0, 0x7f39ecfb3630, 0xc010c26f00,
0xc010be9000)
go/src/pkg/net/http/server.go:1496 +0x19a
appengine_internal.executeRequestSafely(0xc010c26f00, 0xc010be9000)
go/src/pkg/appengine_internal/api_prod.go:280 +0x9f
appengine_internal.(*server).HandleRequest(0x1a57e40, 0xc0109c06c0,
0xc011561900, 0xc011561a80, 0x0, ...)
go/src/pkg/appengine_internal/api_prod.go:214 +0xc35
reflect.Value.call(0x169f3c0, 0x1a57e40, 0x138, 0x172ae10, 0x4, ...)
go/src/pkg/reflect/value.go:474 +0xe23
reflect.Value.Call(0x169f3c0, 0x1a57e40, 0x138, 0x7f39ece23f38, 0x3, ...)
go/src/pkg/reflect/value.go:345 +0xb5

[rsc, 2014-10-07 19:37:35]

*** Submitted as
https://code.google.com/p/go/source/detail?r=117a6816d186&repo=tools ***

dashboard/app: ignore, try not to create partial Commits

It looks like the partial Commits are coming from the build breakages mails.
If you have commit A newer than commit B, then there are two code
paths depending on which reports its build result first.
For slow development, B finishes before A is committed, so when
A notices a failure it checks to see if B was okay.
That code path seems to be okay.
For submit of back-to-back changes, typically A finishes before B,
so when B notices an okay it checks to see if A failed.
That code path seems to zero the Commit for A while
trying to set its FailNotificationSent to true.
It does (did) succeed in setting FailNotificationSent to true,
it just zeroed everything else.

This CL adds code to refuse to do the datastore.Put to
update FailNotificationSent if the Commit info is incomplete.

It also tries to ignore Num=0 records, but that may not be
as important anymore.

LGTM=cmang
R=cmang
CC=golang-codereviews
https://codereview.appspot.com/154080043

[adg, 2014-10-08 02:13:06]

LGTM

Great analysis Russ. Thank you.