Skip to content

Commit

Permalink
Uses GetList within AddListValue and RemoveListValue and adds more te…
Browse files Browse the repository at this point in the history 
…sting
  • Loading branch information
@igooch
igooch committed Oct 26, 2023
1 parent 555ed86 commit 12971d4
Show file tree
Hide file tree
Showing 2 changed files with 137 additions and 88 deletions.
100 changes: 44 additions & 56 deletions pkg/sdkserver/sdkserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,9 @@ func (s *SDKServer) GetList(ctx context.Context, in *alpha.GetListRequest) (*alp
if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
return nil, errors.Errorf("%s not enabled", runtime.FeatureCountsAndLists)
}
if in == nil {
return nil, errors.Errorf("GetListRequest cannot be nil")
}
s.logger.WithField("name", in.Name).Debug("Getting List")

gs, err := s.gameServer()
Expand Down Expand Up @@ -1028,6 +1031,7 @@ func (s *SDKServer) GetList(ctx context.Context, in *alpha.GetListRequest) (*alp
if len(listUpdate.valuesToAppend) != 0 {
protoList.Values = agonesv1.MergeRemoveDuplicates(protoList.Values, listUpdate.valuesToAppend)
}
// Truncates Values to less than or equal to Capacity
if len(protoList.Values) > int(protoList.Capacity) {
protoList.Values = append([]string{}, protoList.Values[:protoList.Capacity]...)
}
Expand All @@ -1047,9 +1051,8 @@ func (s *SDKServer) UpdateList(ctx context.Context, in *alpha.UpdateListRequest)
if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
return nil, errors.Errorf("%s not enabled", runtime.FeatureCountsAndLists)
}

if in.List == nil || in.UpdateMask == nil {
return nil, errors.Errorf("invalid argument. List: %v and UpdateMask %v cannot be nil", in.List, in.UpdateMask)
if in == nil {
return nil, errors.Errorf("UpdateListRequest cannot be nil")
}

name := in.List.Name
Expand Down Expand Up @@ -1089,50 +1092,36 @@ func (s *SDKServer) AddListValue(ctx context.Context, in *alpha.AddListValueRequ
if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
return nil, errors.Errorf("%s not enabled", runtime.FeatureCountsAndLists)
}
if in == nil {
return nil, errors.Errorf("AddListValueRequest cannot be nil")
}
s.logger.WithField("name", in.Name).Debug("Add List Value")

gs, err := s.gameServer()
list, err := s.GetList(ctx, &alpha.GetListRequest{Name: in.Name})
if err != nil {
return nil, err
}

s.gsUpdateMutex.Lock()
defer s.gsUpdateMutex.Unlock()

if list, ok := gs.Status.Lists[in.Name]; ok {
batchList := s.gsListUpdates[in.Name]
// Verify room to add another value
var capacity int64
if batchList.capacitySet != nil {
capacity = *batchList.capacitySet
} else {
capacity = int64(len(list.Values) + len(batchList.valuesToAppend) - len(batchList.valuesToDelete))
}
if list.Capacity <= capacity {
return nil, errors.Errorf("out of range. No available capacity. Current Capacity: %d, List Size: %d", list.Capacity, len(list.Values))
}
// Verify value does not already exist in the list
// TODO: This does not check batched but not yet applied append / remove values. Should we do this?
// (Easy to check not yet applied values, hard to check removed and re-added values.) I'm
// thinking this would be better / easier to do as part of the batch apply update to list, and
// not verify here.
for _, val := range list.Values {
if in.Value == val {
return nil, errors.Errorf("already exists. Value: %s already in List: %s", in.Value, in.Name)
}
}
for _, val := range batchList.valuesToAppend {
if in.Value == val {
return nil, errors.Errorf("already exists. Already received request to remove Value: %s from the List: %s", in.Value, in.Name)
}
// Verify room to add another value
if int(list.Capacity) <= len(list.Values) {
return nil, errors.Errorf("out of range. No available capacity. Current Capacity: %d, List Size: %d", list.Capacity, len(list.Values))
}
// Verify value does not already exist in the list
for _, val := range list.Values {
if in.Value == val {
return nil, errors.Errorf("already exists. Value: %s already in List: %s", in.Value, in.Name)
}
batchList.valuesToAppend = append(batchList.valuesToAppend, in.Value)
s.gsListUpdates[in.Name] = batchList
// Queue up the Update for later batch processing by updateLists.
s.workerqueue.Enqueue(cache.ExplicitKey(updateLists))
return &alpha.List{}, nil
}
return nil, errors.Errorf("not found. %s List not found", in.Name)
list.Values = append(list.Values, in.Value)
batchList := s.gsListUpdates[in.Name]
batchList.valuesToAppend = list.Values
s.gsListUpdates[in.Name] = batchList
// Queue up the Update for later batch processing by updateLists.
s.workerqueue.Enqueue(cache.ExplicitKey(updateLists))
return &alpha.List{}, nil
}

// RemoveListValue collapses all remove a value from a List requests into a single UpdateList request.
Expand All @@ -1144,37 +1133,36 @@ func (s *SDKServer) RemoveListValue(ctx context.Context, in *alpha.RemoveListVal
if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
return nil, errors.Errorf("%s not enabled", runtime.FeatureCountsAndLists)
}

if in == nil {
return nil, errors.Errorf("RemoveListValueRequest cannot be nil")
}
s.logger.WithField("name", in.Name).Debug("Remove List Value")

gs, err := s.gameServer()
list, err := s.GetList(ctx, &alpha.GetListRequest{Name: in.Name})
if err != nil {
return nil, err
}

s.gsUpdateMutex.Lock()
defer s.gsUpdateMutex.Unlock()

if list, ok := gs.Status.Lists[in.Name]; ok {
// Verify value exists in the list
for _, val := range list.Values {
if in.Value != val {
continue
}
// Add value to remove to gsListUpdates map.
batchList := s.gsListUpdates[in.Name]
if batchList.valuesToDelete == nil {
batchList.valuesToDelete = map[string]bool{}
}
batchList.valuesToDelete[in.Value] = true
s.gsListUpdates[in.Name] = batchList
// Queue up the Update for later batch processing by updateLists.
s.workerqueue.Enqueue(cache.ExplicitKey(updateLists))
return &alpha.List{}, nil
// Verify value exists in the list
for _, val := range list.Values {
if in.Value != val {
continue
}
// Add value to remove to gsListUpdates map.
batchList := s.gsListUpdates[in.Name]
if batchList.valuesToDelete == nil {
batchList.valuesToDelete = map[string]bool{}
}
return nil, errors.Errorf("not found. Value: %s not found in List: %s", in.Value, in.Name)
batchList.valuesToDelete[in.Value] = true
s.gsListUpdates[in.Name] = batchList
// Queue up the Update for later batch processing by updateLists.
s.workerqueue.Enqueue(cache.ExplicitKey(updateLists))
return &alpha.List{}, nil
}
return nil, errors.Errorf("not found. %s List not found", in.Name)
return nil, errors.Errorf("not found. Value: %s not found in List: %s", in.Value, in.Name)
}

// updateList updates the Lists in the GameServer's Status with the batched update list requests.
Expand Down
125 changes: 93 additions & 32 deletions pkg/sdkserver/sdkserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1331,22 +1331,54 @@ func TestSDKServerAddListValue(t *testing.T) {
require.NoError(t, err, "Can not parse FeatureCountsAndLists feature")

lists := map[string]agonesv1.ListStatus{
"foo": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
"foo": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(10)},
"bar": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(10)},
"baz": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(10)},
}

fixtures := map[string]struct {
listName string
request *alpha.AddListValueRequest
want agonesv1.ListStatus
updateErr bool
updated bool
listName string
requests []*alpha.AddListValueRequest
want agonesv1.ListStatus
updateErrs []bool
updated bool
}{
"Add value": {
listName: "foo",
request: &alpha.AddListValueRequest{Name: "foo", Value: "five"},
want: agonesv1.ListStatus{Values: []string{"one", "two", "three", "four", "five"}, Capacity: int64(100)},
updateErr: false,
updated: true,
listName: "foo",
requests: []*alpha.AddListValueRequest{{Name: "foo", Value: "five"}},
want: agonesv1.ListStatus{Values: []string{"one", "two", "three", "four", "five"}, Capacity: int64(10)},
updateErrs: []bool{false},
updated: true,
},
"Add multiple values including duplicates": {
listName: "bar",
requests: []*alpha.AddListValueRequest{
{Name: "bar", Value: "five"},
{Name: "bar", Value: "one"},
{Name: "bar", Value: "five"},
{Name: "bar", Value: "zero"},
},
want: agonesv1.ListStatus{Values: []string{"one", "two", "three", "four", "five", "zero"}, Capacity: int64(10)},
updateErrs: []bool{false, true, true, false},
updated: true,
},
"Add multiple values past capacity": {
listName: "baz",
requests: []*alpha.AddListValueRequest{
{Name: "baz", Value: "five"},
{Name: "baz", Value: "six"},
{Name: "baz", Value: "seven"},
{Name: "baz", Value: "eight"},
{Name: "baz", Value: "nine"},
{Name: "baz", Value: "ten"},
{Name: "baz", Value: "eleven"},
},
want: agonesv1.ListStatus{
Values: []string{"one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"},
Capacity: int64(10),
},
updateErrs: []bool{false, false, false, false, false, false, true},
updated: true,
},
}

Expand Down Expand Up @@ -1401,15 +1433,17 @@ func TestSDKServerAddListValue(t *testing.T) {
// check initial value comes through
require.Eventually(t, func() bool {
list, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName})
return cmp.Equal(list.Values, []string{"one", "two", "three", "four"}) && list.Capacity == 100 && err == nil
return cmp.Equal(list.Values, []string{"one", "two", "three", "four"}) && list.Capacity == 10 && err == nil
}, 10*time.Second, time.Second)

// Update the List
_, err = sc.AddListValue(context.Background(), testCase.request)
if testCase.updateErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
for i, req := range testCase.requests {
_, err = sc.AddListValue(context.Background(), req)
if testCase.updateErrs[i] {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
}

got, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName})
Expand Down Expand Up @@ -1446,21 +1480,46 @@ func TestSDKServerRemoveListValue(t *testing.T) {

lists := map[string]agonesv1.ListStatus{
"foo": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
"bar": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
"baz": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
}

fixtures := map[string]struct {
listName string
request *alpha.RemoveListValueRequest
want agonesv1.ListStatus
updateErr bool
updated bool
listName string
requests []*alpha.RemoveListValueRequest
want agonesv1.ListStatus
updateErrs []bool
updated bool
}{
"Remove value": {
listName: "foo",
request: &alpha.RemoveListValueRequest{Name: "foo", Value: "two"},
want: agonesv1.ListStatus{Values: []string{"one", "three", "four"}, Capacity: int64(100)},
updateErr: false,
updated: true,
listName: "foo",
requests: []*alpha.RemoveListValueRequest{{Name: "foo", Value: "two"}},
want: agonesv1.ListStatus{Values: []string{"one", "three", "four"}, Capacity: int64(100)},
updateErrs: []bool{false},
updated: true,
},
"Remove multiple values including duplicates": {
listName: "bar",
requests: []*alpha.RemoveListValueRequest{
{Name: "bar", Value: "two"},
{Name: "bar", Value: "three"},
{Name: "bar", Value: "two"},
},
want: agonesv1.ListStatus{Values: []string{"one", "four"}, Capacity: int64(100)},
updateErrs: []bool{false, false, true},
updated: true,
},
"Remove all values": {
listName: "baz",
requests: []*alpha.RemoveListValueRequest{
{Name: "baz", Value: "three"},
{Name: "baz", Value: "two"},
{Name: "baz", Value: "four"},
{Name: "baz", Value: "one"},
},
want: agonesv1.ListStatus{Values: []string{}, Capacity: int64(100)},
updateErrs: []bool{false, false, false, false},
updated: true,
},
}

Expand Down Expand Up @@ -1519,11 +1578,13 @@ func TestSDKServerRemoveListValue(t *testing.T) {
}, 10*time.Second, time.Second)

// Update the List
_, err = sc.RemoveListValue(context.Background(), testCase.request)
if testCase.updateErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
for i, req := range testCase.requests {
_, err = sc.RemoveListValue(context.Background(), req)
if testCase.updateErrs[i] {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
}

got, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName})
Expand Down

0 comments on commit 12971d4

Please sign in to comment.