Milo buildbot builder page: Add pagnation with cursors.

Milo buildbot builder page: Add pagnation with cursors.

In the UI, backward pages are implemented via memcache, just like luci scheduler
In the API, backward pages are not supported

Note that for the most part we operate around pointers to cursors, to store the state that "the cursor does not exist", ie the end of the query has been reached.

BUG=679931, 700263
Review-Url: https://codereview.chromium.org/2810113002
Committed: https://github.com/luci/luci-go/commit/9fb9880d65325cef1fd8ff4099a1de27b4bf684e

[Ryan Tseng, 2017-04-12 00:24:36]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 00:24:40]

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

[hinoka, 2017-04-12 00:25:18]

Description was changed from

==========
Milo buildbot builder page: Add pagnation with cursors.

BUG=679931,700263
==========

to

==========
Milo buildbot builder page: Add pagnation with cursors.

In the UI, backward pages are implemented via memcache, just like luci scheduler
In the API, backward pages are not supported

BUG=679931,700263
==========

[hinoka, 2017-04-12 00:26:21]

Description was changed from

==========
Milo buildbot builder page: Add pagnation with cursors.

In the UI, backward pages are implemented via memcache, just like luci scheduler
In the API, backward pages are not supported

BUG=679931,700263
==========

to

==========
Milo buildbot builder page: Add pagnation with cursors.

In the UI, backward pages are implemented via memcache, just like luci scheduler
In the API, backward pages are not supported

Note that for the most part we operate around pointers to cursors, to store the
state that "the cursor does not exist", ie the end of the query has been
reached.

BUG=679931,700263
==========

[hinoka, 2017-04-12 00:26:21]

hinoka@chromium.org changed reviewers:
+ nodir@chromium.org

[commit-bot: I haz the power, 2017-04-12 00:30:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 00:30:50]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-04-18 19:34:51]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 19:34:58]

Dry run: 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, 2017-04-18 19:49:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 19:49:08]

Dry run: This issue passed the CQ dry run.

[Ryan Tseng, 2017-04-18 19:53:16]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 19:53:18]

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

[hinoka, 2017-04-18 19:59:25]

hinoka@chromium.org changed reviewers:
+ hinoka@chromium.org

[hinoka, 2017-04-18 19:59:25]

ptal! Demo:
https://1680-8fa48d3-tainted-hinoka-dot-luci-milo-dev.appspot.com/buildbot/ch...

This uses memcache to do backwards traversal, just like luci scheduler

[commit-bot: I haz the power, 2017-04-18 20:00:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 20:00:49]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-04-18 20:27:49]

https://codereview.chromium.org/2810113002/diff/40001/milo/api/resp/builder.go
File milo/api/resp/builder.go (right):

https://codereview.chromium.org/2810113002/diff/40001/milo/api/resp/builder.g...
milo/api/resp/builder.go:68: PrevCursor string
`json:",omitempty"`

https://codereview.chromium.org/2810113002/diff/40001/milo/api/resp/builder.g...
milo/api/resp/builder.go:70: NextCursor string
`json:",omitempty"`

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
File milo/appengine/buildbot/builder.go (right):

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:122: return "v2:cursors:builders:" +
base64.StdEncoding.EncodeToString(blob[:])
are you sure cursors for buildbucket builders will be different? maybe
s/builders/buildbot_builders/ or something

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:122: return "v2:cursors:builders:" +
base64.StdEncoding.EncodeToString(blob[:])
limit has to be included in the cache key. Bug repro:

Go to
https://1680-8fa48d3-tainted-hinoka-dot-luci-milo-dev.appspot.com/buildbot/ch...
I see #23464-#23563. Correct.
Click next
I see #23364-#23463. Correct.
Set page=25
I see #23439-#23463. Correct
Click prev
I see #23539-#23563. Incorrect. Expected: #23464-#23489

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:126: thisKey := key(*nextCursor)
nextKey? it is used only once, consider inlining it, consistent with L141

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:162: cursor *datastore.Cursor)
(*resp.Builder, error) {
consider making cursor a string here. datastore is implementation detail of this
function and so far this function did not expose it, but now it does

this would make http.go return HTTP 500 for a bad cursor, but looks like we
already have this problem: if a builder name is misspelled, we return HTTP 500.
This should be fixed (in a separate CL)

[Ryan Tseng, 2017-04-20 21:36:32]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:36:42]

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

[hinoka, 2017-04-20 21:37:08]

New demo:
https://1683-ab1ded8-tainted-hinoka-cursor-dot-luci-milo-dev.appspot.com/buil...

https://codereview.chromium.org/2810113002/diff/40001/milo/api/resp/builder.go
File milo/api/resp/builder.go (right):

https://codereview.chromium.org/2810113002/diff/40001/milo/api/resp/builder.g...
milo/api/resp/builder.go:68: PrevCursor string
On 2017/04/18 20:27:49, nodir wrote:
> `json:",omitempty"`

Done.

https://codereview.chromium.org/2810113002/diff/40001/milo/api/resp/builder.g...
milo/api/resp/builder.go:70: NextCursor string
On 2017/04/18 20:27:49, nodir wrote:
> `json:",omitempty"`

Done.

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
File milo/appengine/buildbot/builder.go (right):

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:122: return "v2:cursors:builders:" +
base64.StdEncoding.EncodeToString(blob[:])
On 2017/04/18 20:27:49, nodir wrote:
> are you sure cursors for buildbucket builders will be different? maybe
> s/builders/buildbot_builders/ or something

Done.

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:122: return "v2:cursors:builders:" +
base64.StdEncoding.EncodeToString(blob[:])
On 2017/04/18 20:27:49, nodir wrote:
> limit has to be included in the cache key. Bug repro:
> 
> Go to
>
https://1680-8fa48d3-tainted-hinoka-dot-luci-milo-dev.appspot.com/buildbot/ch...
> I see #23464-#23563. Correct.
> Click next
> I see #23364-#23463. Correct.
> Set page=25
> I see #23439-#23463. Correct
> Click prev
> I see #23539-#23563. Incorrect. Expected: #23464-#23489

Blarg.... basically you can't actually go back once you click next -> switch
limit.... I don't think theres a fix for this.  Will just remove prev entirely
in this case and hope no one notices.

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:126: thisKey := key(*nextCursor)
On 2017/04/18 20:27:49, nodir wrote:
> nextKey? it is used only once, consider inlining it, consistent with L141

Done.

https://codereview.chromium.org/2810113002/diff/40001/milo/appengine/buildbot...
milo/appengine/buildbot/builder.go:162: cursor *datastore.Cursor)
(*resp.Builder, error) {
On 2017/04/18 20:27:49, nodir wrote:
> consider making cursor a string here. datastore is implementation detail of
this
> function and so far this function did not expose it, but now it does
> 
> this would make http.go return HTTP 500 for a bad cursor, but looks like we
> already have this problem: if a builder name is misspelled, we return HTTP
500.
> This should be fixed (in a separate CL)

Done.

[commit-bot: I haz the power, 2017-04-20 21:41:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 21:41:33]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/35a61d6bd6a32b10)
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/35a61d6c451d5810)

[nodir, 2017-04-20 21:41:59]

lgtm

[Ryan Tseng, 2017-04-20 22:08:51]

The CQ bit was checked by hinoka@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 22:08:55]

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

[hinoka, 2017-04-20 22:09:25]

The CQ bit was unchecked by hinoka@chromium.org

[hinoka, 2017-04-20 22:09:28]

The CQ bit was checked by hinoka@chromium.org

[hinoka, 2017-04-20 22:09:28]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/2810113002/#ps80001
(title: "train")

[commit-bot: I haz the power, 2017-04-20 22:09:36]

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, 2017-04-20 22:17:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492726168225910,
"parent_rev": "ab1ded87f863007f23e60e56036a4585dd74fb79", "commit_rev":
"9fb9880d65325cef1fd8ff4099a1de27b4bf684e"}

[commit-bot: I haz the power, 2017-04-20 22:17:31]

Description was changed from

==========
Milo buildbot builder page: Add pagnation with cursors.

In the UI, backward pages are implemented via memcache, just like luci scheduler
In the API, backward pages are not supported

Note that for the most part we operate around pointers to cursors, to store the
state that "the cursor does not exist", ie the end of the query has been
reached.

BUG=679931,700263
==========

to

==========
Milo buildbot builder page: Add pagnation with cursors.

In the UI, backward pages are implemented via memcache, just like luci scheduler
In the API, backward pages are not supported

Note that for the most part we operate around pointers to cursors, to store the
state that "the cursor does not exist", ie the end of the query has been
reached.

BUG=679931,700263

Review-Url: https://codereview.chromium.org/2810113002
Committed:
https://github.com/luci/luci-go/commit/9fb9880d65325cef1fd8ff4099a1de27b4bf684e
==========

[commit-bot: I haz the power, 2017-04-20 22:17:32]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-go/commit/9fb9880d65325cef1fd8ff4099a1de27b4bf684e