Milo buildbot builder page: Add pagnation with cursors.

milo/appengine/buildbot/builder.go

Index: milo/appengine/buildbot/builder.go
diff --git a/milo/appengine/buildbot/builder.go b/milo/appengine/buildbot/builder.go
index 8bb9be08e902d1099f4d44a3509e81e135302115..a0c1d7b7b156ca6207b53e04c700c6adfaab0461 100644
--- a/milo/appengine/buildbot/builder.go
+++ b/milo/appengine/buildbot/builder.go
@@ -5,6 +5,8 @@
 package buildbot
 
 import (
+	"crypto/sha1"
+	"encoding/base64"
 	"errors"
 	"fmt"
 	"sort"
@@ -12,6 +14,7 @@ import (
 	"time"
 
 	"github.com/luci/gae/service/datastore"
+	"github.com/luci/gae/service/memcache"
 
 	"github.com/luci/luci-go/common/clock"
 	"github.com/luci/luci-go/common/logging"
@@ -85,8 +88,8 @@ func getBuildSummary(b *buildbotBuild) *resp.BuildSummary {
 // getBuilds fetches all of the recent builds from the .  Note that
 // getBuilds() does not perform ACL checks.
 func getBuilds(
-	c context.Context, masterName, builderName string, finished bool, limit int) (
-	[]*resp.BuildSummary, error) {
+	c context.Context, masterName, builderName string, finished bool, limit int, cursor *datastore.Cursor) (
+	[]*resp.BuildSummary, *datastore.Cursor, error) {
 
 	// TODO(hinoka): Builder specific structs.
 	result := []*resp.BuildSummary{}
@@ -94,19 +97,56 @@ func getBuilds(
 	q = q.Eq("finished", finished)
 	q = q.Eq("master", masterName)
 	q = q.Eq("builder", builderName)
-	if limit != 0 {
-		q = q.Limit(int32(limit))
-	}
 	q = q.Order("-number")
-	buildbots := []*buildbotBuild{}
-	err := getBuildQueryBatcher(c).GetAll(c, q, &buildbots)
+	if cursor != nil {
+		q = q.Start(*cursor)
+	}
+	buildbots, nextCursor, err := runBuildsQuery(c, q, int32(limit))
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	for _, b := range buildbots {
 		result = append(result, getBuildSummary(b))
 	}
-	return result, nil
+	return result, nextCursor, nil
+}
+
+// maybeSetGetCursor is a cheesy way to implement bidirectional paging with forward-only
+// datastore cursor by creating a mapping of nextCursor -> thisCursor
+// in memcache.  maybeSetGetCursor stores the future mapping, then returns prevCursor
+// in the mapping for thisCursor -> prevCursor, if available.
+func maybeSetGetCursor(c context.Context, thisCursor, nextCursor *datastore.Cursor) (*datastore.Cursor, bool) {
+	key := func(c datastore.Cursor) string {
+		// Memcache key limit is 250 bytes, hash our cursor to get under this limit.
+		blob := sha1.Sum([]byte(c.String()))
+		return "v2:cursors:builders:" + base64.StdEncoding.EncodeToString(blob[:])

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

are you sure cursors for buildbucket builders will be different? maybe
s/builders/buildbot_builders/ or something

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

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

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

Done.

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

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.

+	}
+	// Set the next cursor to this cursor mapping, if available.
+	if nextCursor != nil {
+		thisKey := key(*nextCursor)

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

nextKey? it is used only once, consider inlining it, consistent with L141

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

Done.

+		item := memcache.NewItem(c, thisKey)
+		if thisCursor == nil {
+			// Make sure we know it exists, just empty
+			item.SetValue([]byte{})
+		} else {
+			item.SetValue([]byte((*thisCursor).String()))
+		}
+		item.SetExpiration(24 * time.Hour)
+		memcache.Set(c, item)
+	}
+	// Try to get the last cursor, if valid and available.
+	if thisCursor == nil {
+		return nil, false
+	}
+	if item, err := memcache.GetKey(c, key(*thisCursor)); err == nil {
+		if len(item.Value()) == 0 {
+			return nil, true
+		}
+		if prevCursor, err := datastore.DecodeCursor(c, string(item.Value())); err == nil {
+			return &prevCursor, true
+		}
+	}
+	return nil, false
 }
 
 var errMasterNotFound = errors.New(
@@ -117,7 +157,10 @@ var errNotAuth = errors.New("You are not authenticated, try logging in")
 // This gets:
 // * Current Builds from querying the master json from the datastore.
 // * Recent Builds from a cron job that backfills the recent builds.
-func builderImpl(c context.Context, masterName, builderName string, limit int) (*resp.Builder, error) {
+func builderImpl(
+	c context.Context, masterName, builderName string, limit int,
+	cursor *datastore.Cursor) (*resp.Builder, error) {

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

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)

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

Done.

+
 	result := &resp.Builder{
 		Name: builderName,
 	}
@@ -167,11 +210,23 @@ func builderImpl(c context.Context, masterName, builderName string, limit int) (
 	}
 
 	// This is CPU bound anyways, so there's no need to do this in parallel.
-	finishedBuilds, err := getBuilds(c, masterName, builderName, true, limit)
+	finishedBuilds, nextCursor, err := getBuilds(c, masterName, builderName, true, limit, cursor)
 	if err != nil {
 		return nil, err
 	}
-	currentBuilds, err := getBuilds(c, masterName, builderName, false, 0)
+	if prevCursor, ok := maybeSetGetCursor(c, cursor, nextCursor); ok {
+		if prevCursor == nil {
+			// Magic string to signal display prev without cursor
+			result.PrevCursor = "EMPTY"
+		} else {
+			result.PrevCursor = (*prevCursor).String()
+		}
+	}
+	if nextCursor != nil {
+		result.NextCursor = (*nextCursor).String()
+	}
+	// Cursor is not needed for current builds.
+	currentBuilds, _, err := getBuilds(c, masterName, builderName, false, 0, nil)
 	if err != nil {
 		return nil, err
 	}