Repository: googleforgames/open-match
Pull request #773 (2019-09-03 20:39:05)

Have filter tickets take pool instead of list of filters
Part of the work for https://github.com/googleforgames/open-match/issues/681

This changes the API of FilterTickets to take a pool instead of filter list.  Following the API
in the proposal, different filter types will be different fields on the Pool message.

sawagh: _This changes the API of FilterTickets to take a pool instead of filter list._ 

This PR does not change any external API in this change right? this just changes the store API interface which is internal only - correct?
Laremere: > _This changes the API of FilterTickets to take a pool instead of filter list._
> 
> This PR does not change any external API in this change right? this just changes the store API interface which is internal only - correct?

Correct.

Patch:
diff --git a/go.mod b/go.mod
index efd6d11f..43d817aa 100644
--- a/go.mod
+++ b/go.mod
@@ -27,7 +27,6 @@ require (
 	github.com/alicebob/miniredis/v2 v2.8.1-0.20190618082157-e29950035715
 	github.com/cenkalti/backoff v2.1.1+incompatible
 	github.com/fsnotify/fsnotify v1.4.7
-	github.com/go-logfmt/logfmt v0.4.0 // indirect
 	github.com/golang/protobuf v1.3.2
 	github.com/gomodule/redigo v1.7.1-0.20190322064113-39e2c31b7ca3
 	github.com/google/gofuzz v1.0.0 // indirect
diff --git a/go.sum b/go.sum
index 5233fc31..601df654 100644
--- a/go.sum
+++ b/go.sum
@@ -221,6 +221,7 @@ github.com/yuin/gopher-lua v0.0.0-20190206043414-8bfc7677f583/go.mod h1:gqRgreBU
 go.etcd.io/bbolt v1.3.2/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU=
 go.opencensus.io v0.20.1/go.mod h1:6WKK9ahsWS3RSO+PY9ZHZUfv2irvY6gN279GOPZjmmk=
 go.opencensus.io v0.21.0/go.mod h1:mSImk1erAIZhrmZN+AvHh14ztQfjbGwt4TtuofqLduU=
+go.opencensus.io v0.22.0 h1:C9hSCOW830chIVkdja34wa6Ky+IzWllkUinR+BtRZd4=
 go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8=
 go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
 go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
diff --git a/internal/app/mmlogic/mmlogic_service.go b/internal/app/mmlogic/mmlogic_service.go
index 53ed0b34..563cce32 100644
--- a/internal/app/mmlogic/mmlogic_service.go
+++ b/internal/app/mmlogic/mmlogic_service.go
@@ -43,12 +43,12 @@ type mmlogicService struct {
 // GetPoolTickets gets the list of Tickets that match every Filter in the
 // specified Pool.
 func (s *mmlogicService) QueryTickets(req *pb.QueryTicketsRequest, responseServer pb.MmLogic_QueryTicketsServer) error {
-	if req.GetPool() == nil {
+	pool := req.GetPool()
+	if pool == nil {
 		return status.Error(codes.InvalidArgument, ".pool is required")
 	}
 
 	ctx := responseServer.Context()
-	poolFilters := req.GetPool().GetFilters()
 	pSize := getPageSize(s.cfg)
 
 	callback := func(tickets []*pb.Ticket) error {
@@ -60,12 +60,12 @@ func (s *mmlogicService) QueryTickets(req *pb.QueryTicketsRequest, responseServe
 		return nil
 	}
 
-	return doQueryTickets(ctx, poolFilters, pSize, callback, s.store)
+	return doQueryTickets(ctx, pool, pSize, callback, s.store)
 }
 
-func doQueryTickets(ctx context.Context, filters []*pb.Filter, pageSize int, sender func(tickets []*pb.Ticket) error, store statestore.Service) error {
+func doQueryTickets(ctx context.Context, pool *pb.Pool, pageSize int, sender func(tickets []*pb.Ticket) error, store statestore.Service) error {
 	// Send requests to the storage service
-	err := store.FilterTickets(ctx, filters, pageSize, sender)
+	err := store.FilterTickets(ctx, pool, pageSize, sender)
 	if err != nil {
 		logger.WithError(err).Error("Failed to retrieve result from storage service.")
 		return err


Test patch:
diff --git a/internal/app/backend/backend_service_test.go b/internal/app/backend/backend_service_test.go
index b45fde3e..cc4019c8 100644
--- a/internal/app/backend/backend_service_test.go
+++ b/internal/app/backend/backend_service_test.go
@@ -232,7 +232,10 @@ func TestDoAssignTickets(t *testing.T) {
 				}
 				// Make sure tickets are correctly indexed.
 				var wantFilteredTickets []*pb.Ticket
-				err := store.FilterTickets(ctx, []*pb.Filter{{Attribute: fakeProperty, Min: 0, Max: 3}}, 10, func(filterTickets []*pb.Ticket) error {
+				pool := &pb.Pool{
+					Filters: []*pb.Filter{{Attribute: fakeProperty, Min: 0, Max: 3}},
+				}
+				err := store.FilterTickets(ctx, pool, 10, func(filterTickets []*pb.Ticket) error {
 					wantFilteredTickets = filterTickets
 					return nil
 				})
@@ -275,7 +278,10 @@ func TestDoAssignTickets(t *testing.T) {
 
 				// Make sure tickets are deindexed after assignment
 				var wantFilteredTickets []*pb.Ticket
-				store.FilterTickets(ctx, []*pb.Filter{{Attribute: fakeProperty, Min: 0, Max: 2}}, 10, func(filterTickets []*pb.Ticket) error {
+				pool := &pb.Pool{
+					Filters: []*pb.Filter{{Attribute: fakeProperty, Min: 0, Max: 2}},
+				}
+				store.FilterTickets(ctx, pool, 10, func(filterTickets []*pb.Ticket) error {
 					wantFilteredTickets = filterTickets
 					return nil
 				})
diff --git a/internal/app/mmlogic/mmlogic_service_test.go b/internal/app/mmlogic/mmlogic_service_test.go
index a2037987..0e88f7e2 100644
--- a/internal/app/mmlogic/mmlogic_service_test.go
+++ b/internal/app/mmlogic/mmlogic_service_test.go
@@ -58,7 +58,7 @@ func TestDoQueryTickets(t *testing.T) {
 	tests := []struct {
 		description string
 		sender      func(tickets []*pb.Ticket) error
-		filters     []*pb.Filter
+		pool        *pb.Pool
 		pageSize    int
 		action      func(context.Context, *testing.T, statestore.Service)
 		wantErr     error
@@ -67,11 +67,13 @@ func TestDoQueryTickets(t *testing.T) {
 		{
 			"expect empty response from an empty store",
 			senderGenerator(nil),
-			[]*pb.Filter{
-				{
-					Attribute: attribute1,
-					Min:       0,
-					Max:       10,
+			&pb.Pool{
+				Filters: []*pb.Filter{
+					{
+						Attribute: attribute1,
+						Min:       0,
+						Max:       10,
+					},
 				},
 			},
 			100,
@@ -82,11 +84,13 @@ func TestDoQueryTickets(t *testing.T) {
 		{
 			"expect tickets with attribute1 value in range of [0, 10] (inclusively)",
 			senderGenerator(nil),
-			[]*pb.Filter{
-				{
-					Attribute: attribute1,
-					Min:       0,
-					Max:       10,
+			&pb.Pool{
+				Filters: []*pb.Filter{
+					{
+						Attribute: attribute1,
+						Min:       0,
+						Max:       10,
+					},
 				},
 			},
 			100,
@@ -105,11 +109,13 @@ func TestDoQueryTickets(t *testing.T) {
 		{
 			"expect error from canceled context",
 			senderGenerator(fakeErr),
-			[]*pb.Filter{
-				{
-					Attribute: attribute1,
-					Min:       0,
-					Max:       10,
+			&pb.Pool{
+				Filters: []*pb.Filter{
+					{
+						Attribute: attribute1,
+						Min:       0,
+						Max:       10,
+					},
 				},
 			},
 			100,
@@ -135,7 +141,7 @@ func TestDoQueryTickets(t *testing.T) {
 			ctx := utilTesting.NewContext(t)
 
 			test.action(ctx, t, store)
-			assert.Equal(t, test.wantErr, doQueryTickets(ctx, test.filters, test.pageSize, test.sender, store))
+			assert.Equal(t, test.wantErr, doQueryTickets(ctx, test.pool, test.pageSize, test.sender, store))
 			for _, wantTicket := range test.wantTickets {
 				assert.Contains(t, actualTickets, wantTicket)
 			}
diff --git a/internal/statestore/instrumented.go b/internal/statestore/instrumented.go
index 7f67a4ae..3c3333f5 100644
--- a/internal/statestore/instrumented.go
+++ b/internal/statestore/instrumented.go
@@ -85,8 +85,8 @@ func (is *instrumentedService) DeindexTicket(ctx context.Context, id string) err
 //  "testplayer1": {"ranking" : 56, "loyalty_level": 4},
 //  "testplayer2": {"ranking" : 50, "loyalty_level": 3},
 // }
-func (is *instrumentedService) FilterTickets(ctx context.Context, filters []*pb.Filter, pageSize int, callback func([]*pb.Ticket) error) error {
-	return is.s.FilterTickets(ctx, filters, pageSize, func(t []*pb.Ticket) error {
+func (is *instrumentedService) FilterTickets(ctx context.Context, pool *pb.Pool, pageSize int, callback func([]*pb.Ticket) error) error {
+	return is.s.FilterTickets(ctx, pool, pageSize, func(t []*pb.Ticket) error {
 		telemetry.IncrementCounterN(ctx, mStateStoreFilterTickets, len(t))
 		return callback(t)
 	})
diff --git a/internal/statestore/public.go b/internal/statestore/public.go
index 522bd447..a4c16dd5 100644
--- a/internal/statestore/public.go
+++ b/internal/statestore/public.go
@@ -43,7 +43,7 @@ type Service interface {
 	DeindexTicket(ctx context.Context, id string) error
 
 	// FilterTickets returns the Ticket ids for the Tickets meeting the specified filtering criteria.
-	FilterTickets(ctx context.Context, filters []*pb.Filter, pageSize int, callback func([]*pb.Ticket) error) error
+	FilterTickets(ctx context.Context, pool *pb.Pool, pageSize int, callback func([]*pb.Ticket) error) error
 
 	// UpdateAssignments update the match assignments for the input ticket ids
 	UpdateAssignments(ctx context.Context, ids []string, assignment *pb.Assignment) error
diff --git a/internal/statestore/redis.go b/internal/statestore/redis.go
index f7c18aee..79f07851 100644
--- a/internal/statestore/redis.go
+++ b/internal/statestore/redis.go
@@ -395,7 +395,7 @@ func (rb *redisBackend) DeindexTicket(ctx context.Context, id string) error {
 //  "testplayer1": {"ranking" : 56, "loyalty_level": 4},
 //  "testplayer2": {"ranking" : 50, "loyalty_level": 3},
 // }
-func (rb *redisBackend) FilterTickets(ctx context.Context, filters []*pb.Filter, pageSize int, callback func([]*pb.Ticket) error) error {
+func (rb *redisBackend) FilterTickets(ctx context.Context, pool *pb.Pool, pageSize int, callback func([]*pb.Ticket) error) error {
 	var err error
 	var redisConn redis.Conn
 	var ticketBytes [][]byte
@@ -411,7 +411,7 @@ func (rb *redisBackend) FilterTickets(ctx context.Context, filters []*pb.Filter,
 	idSet := make([]string, 0)
 
 	// For each filter, do a range query to Redis on Filter.Attribute
-	for i, filter := range filters {
+	for i, filter := range pool.Filters {
 		// Time Complexity O(logN + M), where N is the number of elements in the attribute set
 		// and M is the number of entries being returned.
 		// TODO: discuss if we need a LIMIT for # of queries being returned
diff --git a/internal/statestore/redis_test.go b/internal/statestore/redis_test.go
index 7fc38938..f6878617 100644
--- a/internal/statestore/redis_test.go
+++ b/internal/statestore/redis_test.go
@@ -18,6 +18,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"testing"
+	"time"
+
 	miniredis "github.com/alicebob/miniredis/v2"
 	"github.com/rs/xid"
 	"github.com/spf13/viper"
@@ -30,8 +33,6 @@ import (
 	utilTesting "open-match.dev/open-match/internal/util/testing"
 	"open-match.dev/open-match/pkg/pb"
 	"open-match.dev/open-match/pkg/structs"
-	"testing"
-	"time"
 )
 
 func TestStatestoreSetup(t *testing.T) {
@@ -123,7 +124,13 @@ func TestIgnoreLists(t *testing.T) {
 
 	verifyTickets := func(service Service, expectLen int) {
 		var results []*pb.Ticket
-		service.FilterTickets(ctx, []*pb.Filter{{Attribute: "testindex1", Min: 0, Max: 10}, {Attribute: "testindex2", Min: 0, Max: 10}}, 100, func(tickets []*pb.Ticket) error {
+		pool := &pb.Pool{
+			Filters: []*pb.Filter{
+				{Attribute: "testindex1", Min: 0, Max: 10},
+				{Attribute: "testindex2", Min: 0, Max: 10},
+			},
+		}
+		service.FilterTickets(ctx, pool, 100, func(tickets []*pb.Ticket) error {
 			results = tickets
 			return nil
 		})
@@ -183,20 +190,22 @@ func TestTicketIndexing(t *testing.T) {
 
 	found := make(map[string]struct{})
 
-	filters := []*pb.Filter{
-		{
-			Attribute: "testindex1",
-			Min:       2.5,
-			Max:       8.5,
-		},
-		{
-			Attribute: "testindex2",
-			Min:       0.49,
-			Max:       0.51,
+	pool := &pb.Pool{
+		Filters: []*pb.Filter{
+			{
+				Attribute: "testindex1",
+				Min:       2.5,
+				Max:       8.5,
+			},
+			{
+				Attribute: "testindex2",
+				Min:       0.49,
+				Max:       0.51,
+			},
 		},
 	}
 
-	err = service.FilterTickets(ctx, filters, 2, func(tickets []*pb.Ticket) error {
+	err = service.FilterTickets(ctx, pool, 2, func(tickets []*pb.Ticket) error {
 		assert.True(len(tickets) <= 2)
 		for _, ticket := range tickets {
 			found[ticket.Id] = struct{}{}