test-results: package frontend: Add delete keys task queue

test-results: package frontend: Add delete keys task queue

* Register handlers in init()
* Upload handler
  * Add delete keys handler and task queue call
  * Bug fixes
  * Add function: extractBuildNumber
* upload_test.go: Add tests

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/1952d7c4b68bf49967e601d816c0ea5fd6908a58

[nishanths, 2016-08-16 05:03:38]

Description was changed from

==========
test-results: pacakge frontend: Add delete keys task queue

* Register handlers in init()
* Upload handler
  * Add delete keys handler and task queue call
  * Bug fixes
  * Add function: extractBuildNumber
* upload_test.go: Add tests

BUG=
==========

to

==========
test-results: package frontend: Add delete keys task queue

* Register handlers in init()
* Upload handler
  * Add delete keys handler and task queue call
  * Bug fixes
  * Add function: extractBuildNumber
* upload_test.go: Add tests

BUG=
==========

[nishanths, 2016-08-16 05:03:38]

nishanths@google.com changed reviewers:
+ estaab@chromium.org, martiniss@chromium.org, vadim@chromium.org

[nishanths, 2016-08-16 06:35:46]

nishanths@google.com changed reviewers:
+ vadimsh@chromium.org
- vadim@chromium.org

[Vadim Sh., 2016-08-16 18:56:13]

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
File go/src/infra/appengine/test-results/frontend/handlers.go (right):

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/handlers.go:74:
w.WriteHeader(http.StatusInternalServerError)
better to return 200 here
otherwise Task Queue service would retry the task again (with same body, so it
will fail again).

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
File go/src/infra/appengine/test-results/frontend/testdata/index.yaml (right):

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/testdata/index.yaml:1: indexes:
I assume it matches Python version identically?

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
File go/src/infra/appengine/test-results/frontend/upload.go (right):

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:168: go
builderstate.Update(c, p.Master, p.Builder, p.TestType, time.Now().UTC())
you need to wait for these goroutines to finish somewhere, since GAE doesn't
guarantee that it won't kill the process right after handling the request.

Also, it's good to check errors.

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:170:
taskqueue.NewPOSTTask("/internal/monitoring/upload", url.Values{
does this handler exist?

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:375: return se
beware that RunInTransaction can return an error on its own, even if the
callback is successful (if transaction can't be committed).

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:380: },
&datastore.TransactionOptions{XG: true})
how many entity groups does this transaction modify?

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:452: return err
statusError{err, http.StatusInternalServerError} ?

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:454: if err :=
deleteKeys(c, tf.OldDataKeys); err != nil {
ignoring this error?

[nishanths, 2016-08-16 20:35:13]

Addressed Vadim's comments!

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
File go/src/infra/appengine/test-results/frontend/handlers.go (right):

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/handlers.go:74:
w.WriteHeader(http.StatusInternalServerError)
On 2016/08/16 18:56:13, Vadim Sh. wrote:
> better to return 200 here
> otherwise Task Queue service would retry the task again (with same body, so it
> will fail again).

Done, thanks!

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
File go/src/infra/appengine/test-results/frontend/testdata/index.yaml (right):

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/testdata/index.yaml:1: indexes:
On 2016/08/16 18:56:13, Vadim Sh. wrote:
> I assume it matches Python version identically?

Fixed, should have been a symlink.

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
File go/src/infra/appengine/test-results/frontend/upload.go (right):

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:168: go
builderstate.Update(c, p.Master, p.Builder, p.TestType, time.Now().UTC())
On 2016/08/16 18:56:13, Vadim Sh. wrote:
> you need to wait for these goroutines to finish somewhere, since GAE doesn't
> guarantee that it won't kill the process right after handling the request.
> 
> Also, it's good to check errors.

Done.

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:170:
taskqueue.NewPOSTTask("/internal/monitoring/upload", url.Values{
On 2016/08/16 18:56:13, Vadim Sh. wrote:
> does this handler exist?

It exists in the python implementation.

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:375: return se
On 2016/08/16 18:56:13, Vadim Sh. wrote:
> beware that RunInTransaction can return an error on its own, even if the
> callback is successful (if transaction can't be committed).

in that case, the error will be propagated at line 163 and handled at line 127
(let me know if i'm missing something.)

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:380: },
&datastore.TransactionOptions{XG: true})
On 2016/08/16 18:56:13, Vadim Sh. wrote:
> how many entity groups does this transaction modify?

Two entity groups. (1) TestFile (2) DataEntry

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:452: return err
On 2016/08/16 18:56:13, Vadim Sh. wrote:
> statusError{err, http.StatusInternalServerError} ?

Done.

https://codereview.chromium.org/2250043002/diff/1/go/src/infra/appengine/test...
go/src/infra/appengine/test-results/frontend/upload.go:454: if err :=
deleteKeys(c, tf.OldDataKeys); err != nil {
On 2016/08/16 18:56:13, Vadim Sh. wrote:
> ignoring this error?

yes, only logging the error. 

Returning an HTTP error 500 to the client may result in the client retrying the
POST request, which would not be idempotent.

[Vadim Sh., 2016-08-16 21:27:12]

https://codereview.chromium.org/2250043002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/frontend/upload.go (right):

https://codereview.chromium.org/2250043002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/frontend/upload.go:195: return nil
no reporting errors back to the user?

[nishanths, 2016-08-16 21:29:54]

https://codereview.chromium.org/2250043002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/frontend/upload.go (right):

https://codereview.chromium.org/2250043002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/frontend/upload.go:195: return nil
On 2016/08/16 21:27:12, Vadim Sh. wrote:
> no reporting errors back to the user?

These errors are not critical to the POST request.
(The python implementation also does not report these errors, but let me know if
we should be.)

[Vadim Sh., 2016-08-16 21:34:47]

https://codereview.chromium.org/2250043002/diff/40001/go/src/infra/appengine/...
File go/src/infra/appengine/test-results/frontend/upload.go (right):

https://codereview.chromium.org/2250043002/diff/40001/go/src/infra/appengine/...
go/src/infra/appengine/test-results/frontend/upload.go:195: return nil
On 2016/08/16 21:29:54, nishanths wrote:
> On 2016/08/16 21:27:12, Vadim Sh. wrote:
> > no reporting errors back to the user?
> 
> These errors are not critical to the POST request.
> (The python implementation also does not report these errors, but let me know
if
> we should be.)

I have no idea how it all works on high level, I just review local details :)

Beware that in Python stuff (datastore requests) can throw DeadlineExceeded and
other exceptions at any time, that (unless caught) result in HTTP 500, that is
usually retried by the client. In Go these exceptions manifest as errors from
datastore.Put etc, so ignoring them is not the same as not explicitly checking
exceptions in Python.

[nishanths, 2016-08-16 21:47:50]

On 2016/08/16 21:34:47, Vadim Sh. wrote:
>
https://codereview.chromium.org/2250043002/diff/40001/go/src/infra/appengine/...
> File go/src/infra/appengine/test-results/frontend/upload.go (right):
> 
>
https://codereview.chromium.org/2250043002/diff/40001/go/src/infra/appengine/...
> go/src/infra/appengine/test-results/frontend/upload.go:195: return nil
> On 2016/08/16 21:29:54, nishanths wrote:
> > On 2016/08/16 21:27:12, Vadim Sh. wrote:
> > > no reporting errors back to the user?
> > 
> > These errors are not critical to the POST request.
> > (The python implementation also does not report these errors, but let me
know
> if
> > we should be.)
> 
> I have no idea how it all works on high level, I just review local details :)
> 
> Beware that in Python stuff (datastore requests) can throw DeadlineExceeded
and
> other exceptions at any time, that (unless caught) result in HTTP 500, that is
> usually retried by the client. In Go these exceptions manifest as errors from
> datastore.Put etc, so ignoring them is not the same as not explicitly checking
> exceptions in Python.

Thanks for explaining!

All critical datastore.Put operations in the Go app are being checked for errors
and returned to the client. The two goroutine calls here for which the errors
are not being returned to the client do the following:

(1) update last known builder update time 
(2) tsmon and eventmon monitoring

(1) is not critical to the user because there is a cron job that updates this
information every 30 minutes anyway. (2) is not critical to the user because it
is just internal monitoring. Even if the python app would return HTTP 500 due to
uncaught exceptions in the above 2 calls, we should not be doing so in the Go
app imo.

[Vadim Sh., 2016-08-16 21:52:19]

ok, lgtm then

[nishanths, 2016-08-17 00:41:16]

The CQ bit was checked by nishanths@google.com

[nishanths, 2016-08-17 00:41:16]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2250043002/#ps60001
(title: "(Rebase)")

[commit-bot: I haz the power, 2016-08-17 00:41:19]

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

[commit-bot: I haz the power, 2016-08-17 00:54:38]

Description was changed from

==========
test-results: package frontend: Add delete keys task queue

* Register handlers in init()
* Upload handler
  * Add delete keys handler and task queue call
  * Bug fixes
  * Add function: extractBuildNumber
* upload_test.go: Add tests

BUG=
==========

to

==========
test-results: package frontend: Add delete keys task queue

* Register handlers in init()
* Upload handler
  * Add delete keys handler and task queue call
  * Bug fixes
  * Add function: extractBuildNumber
* upload_test.go: Add tests

BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/1952d7c4b68bf49967e601d816c0e...
==========

[commit-bot: I haz the power, 2016-08-17 00:54:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/1952d7c4b68bf49967e601d816c0e...