Frontend acknowledge backfill #1293

aLekSer · 2020-11-25T13:29:44Z

Add AcknowledgeBackfill() request handler.

What this PR does / Why we need it:
Create AssignmentGroup for all associated tickets and use Redis stateStore, not backend to assign all tickets.

Which issue(s) this PR fixes:
Work on #1240

Special notes for your reviewer:
Contains part of #1279 as well as #1288 , so check only latest commit when viewing files changes. Namely:
9b5cfde
AcknowledgeBackfill do not update Generation, unlike other UpdateBackfill methods (from GS and Frontend).

Store Backfills in a sorted set to retrieve all expired backfills by time.

It has the same input as output of GetExpiredBackfillIDs function. Some backfillIDs can be acknowledged in between,But this should not affect the work of cleanup service.

Used CreateBackfill implementation from another PR.

internal/app/frontend/frontend_service.go

aLekSer · 2020-11-26T14:34:08Z

CreateBackfill() should also query AcknowledgeBackfill in Redis, just to set initial initial timestamp.
Frontend AcknowledgeBackfill request, should first check if Backfill exists in Redis, otherwise AcknowledgeBackfill can create a new record in sorted set for non-existent Backfill.

Return back missing test after the merge.

Needed in 2 tests - AcknowledgeBackfill and ProposedBackfillCreate.

akremsa · 2020-12-09T10:54:53Z

internal/testing/e2e/backfill_test.go

-		panic(err)
-	}
-	return result
+	actual, err := om.Frontend().GetBackfill(ctx, &pb.GetBackfillRequest{BackfillId: bfID})


Why do you test GetBackfill here?
The name of the function is createMatchWithBackfill and you are passing backfill as a parameter.
I think if you want to test GetBackfill you should do it out of this method in order to not overload one with a lot of code. createMatchWithBackfill does more than a simple create match step here.

This is a refactor of @yeukovichd test. Now it is used in 3 place, so that we can associate ticket with backfill using a match proposal.

One more additional check is not bad. So I will not remove this line.

test fixes.

akremsa · 2020-12-09T12:42:41Z

internal/app/frontend/frontend_service.go

+	if req.GetBackfillId() == "" {
+		return nil, status.Errorf(codes.InvalidArgument, ".BackfillId is required")
+	}
+	if req.GetAssignment().Connection == "" {


If Assignment is nil you will fail here with invalid memory address or nil pointer dereference error

akremsa · 2020-12-09T12:47:58Z

internal/testing/e2e/backfill_test.go

+
+	ticketID := createMatchWithBackfill(ctx, om, createdBf, t)
+	conn := "127.0.0.1:4242"
+	getBF, err := om.Frontend().AcknowledgeBackfill(ctx, &pb.AcknowledgeBackfillRequest{BackfillId: createdBf.Id, Assignment: &pb.Assignment{Connection: conn, Extensions: map[string]*any.Any{


tests with parameters validation are missing. seems you are testing only a happy path.

Well I think parameters validation can form a new test - frontend_service_test.go

internal/app/frontend/frontend_service.go

akremsa · 2020-12-09T20:26:21Z

internal/testing/e2e/backfill_test.go

@@ -132,20 +131,39 @@ func TestBackfillFrontendLifecycle(t *testing.T) {
 	require.Nil(t, get)
 }

-func TestProposedBackfillCreate(t *testing.T) {
-	ctx := context.Background()
+func TestAcknowledgeBackfill(t *testing.T) {


Actually, this test does not cover AcknowledgeBackfill at all, otherwise, you would notice that you do not call the Acknowledge backfill method from a statestore. This test should be completely rewritten.

This is E2E test, so it is hard to test this.
I know that there is an additional Redis state change - this would be covered in a Unit test.
It covers one of main effects of acknowledgment - all tickets get assigned.

Ok, I see why you can not cover AcknowledgeBackfill step in e2e, you still can do that in unit test to check the redis state, that is what I meant. What do you think?

aLekSer · 2020-12-09T23:03:30Z

@akremsa Addressed your comments. Tests which are flaky on CI are TestProposedBackfillCreate and TestProposedBackfillUpdate

internal/app/frontend/frontend_service_test.go

When FetchMatches takes more than 200 ms pendingReleaseTimeout Ticket got expired and no Tickets can be queried thereafter.

Laremere · 2020-12-10T21:39:49Z

internal/app/frontend/frontend_service.go

+	if req.GetAssignment() == nil {
+		return nil, status.Errorf(codes.InvalidArgument, ".Assignment is required")
+	}
+	if req.GetAssignment().Connection == "" {


Connection is not a required field on assignments.

internal/app/frontend/frontend_service.go

Laremere · 2020-12-10T21:56:05Z

internal/app/frontend/frontend_service.go

+			return nil, err
+		}
+	}
+	return bf, nil
 }


After the tickets are assigned, they need to be removed from the associated ticket list? Otherwise they'll be assigned every acknowledge call?

Updated: now removing only assigned tickets from Associated tickets list.

And inside the mutex section

Add Mutex lock, UpdateBackfill accordingly after UpdateAssignments call.

aLekSer · 2020-12-11T21:04:23Z

All comments are resolved

thisisnotapril · 2020-12-14T23:04:13Z

@HazWard @calebatwd would love to have y'all review

davidedmonds

Looking good from over here - seems all the comments have been addressed as well.

sawagh · 2020-12-17T20:58:55Z

internal/app/frontend/frontend_service.go

+		return nil, status.Errorf(codes.InvalidArgument, ".Assignment is required")
+	}
+
+	m := s.store.NewMutex(req.GetBackfillId())


Am curious what this mutex is protecting? I assume it protects the backfill structure itself from getting updated when we are doing this. However, I see that the UpdateAssignments call is independent of the backfill structure - so you could potentially have another parallel UpdateAssignments stomp on the tickets when this method is doing so too. Is that OK?

Note that on line 406 we execute UpdateBackfill and Backfill internal protobuf format includes also ticketed associated with this Backfill. So this lock is necessary here.
https://github.com/googleforgames/open-match/pull/1293/files#diff-fbf6a8aad66158048250071262ad0c5fd8c08c9034840c5802784888408881f3R406

I do agree that the lock is necessary here prevent the backfill from being modified from concurrent calls. I am not sure if it is sufficient. Can you explain how it protects the UpdateAssignments call from modifying the tickets while they are being updated from the AcknowledgeBackfill call?

Tickets are immutable, so we don't need to keep track of their state. They can only be assigned, but searchFields could not be updated, this is the main difference with Backfill which has generation (version) number.
That's why no changes to that end is needed.

I did not mention updating the searchFields at all.

TicketA is a part of BackfillA. You receive a call to AcknowledgeBackfill for BackfillA - and are updating the assignment on the TicketA by making UpdateAssignments call - Line 389.

Concurrently, the Backend receives AssignTickets method setting Assignment for TicketA to a different assignment.

Is there anything that prevents the AssignTickets method from calling UpdateAssignments call on TicketA concurrently with AcknowledgeBackfill calliong UpdateAssignments for TicketA?

Actually this is not possible to have one ticket being associated to two backfills or used in a normal match. Evaluator would not make this happen. Only one match would be selected with a ticketA and later on it would be moved into pendingRelease tickets not visible by QueryTickets later on. We are working on scale tests it will let us know if concurrent issues like this would arise.

sawagh · 2020-12-17T21:03:07Z

internal/app/frontend/frontend_service.go

+			return nil, err
+		}
+
+		// all assigned tickets should be removed from the associatedTickets


(For my understanding)
I see us select all tickets on the backfill in associatedTickets (from call to s.store.GetBackfill), We then assign all those tickets the assignment in s.store.UpdateAssignments - which returns us tickets. We later update the backfill with the delta between associatedTickets and this 'tickets' which are essentially unassigned.

Why does this 'unassigned tickets' exist? didnt we intend to update assignment on all the tickets when calling the UpdateAssignment call?

Note that UpdateAssignment can return not the full list and some tickets are left unassigned.

Looking at the code in UpdateAssignment, any ticket left unassigned is because it was not found. (See: AssignmentFailure_TICKET_NOT_FOUND). So currently, any ticket that is returned as not found from UpdateAssignment is still left on the Backfill? We should atleast log the tickets that we leave unassigned here as UpdateAssignment returns a list of errors along with a list of assigned tickets (currently we ignore the errors list)

I have added logging of the errors on updateAssignment request.
We decided to remove all tickets after UpdateAssignment() call, since those unassignedTickets are not found.
We can add this unassigned tickets back in a separate PR if a need arise.

internal/app/frontend/frontend_service.go

aLekSer · 2020-12-23T13:05:14Z

Actually I have chosen probably not the best name for statestore function AcknowledgeBackfill(). I will rename statestore function to UpdateAcknowledgmentTimestamp().

New function name seems more reasonable as it do only Redis timestamp updates, updateAcknowledgmentTimestamp.

internal/statestore/backfill_test.go

internal/statestore/backfill.go

aLekSer · 2020-12-23T14:50:07Z

Simplified the e2e/backfill_test.go review process, refactoring of the same code which is part of createMatchWithBackfill() I decided to move out of this PR.

aLekSer · 2020-12-23T20:40:28Z

@sawagh I have applied all your comments. PTAL

We need to deindex tickets and add error logging for all NotFound tickets.

aLekSer · 2020-12-24T13:09:40Z

@sawagh I have added error logging with Test case containing one of two tickets got deleted before the AcknowledgeBackfill call.

sawagh

If we dont resolve the concurrency comment, I am ok checking this in and tracking that in a separate issue.

aLekSer added 3 commits November 23, 2020 11:26

StateStore: add LastAcknowledge timestamp to Backfill

74eacae

Store Backfills in a sorted set to retrieve all expired backfills by time.

Add DeleteExpiredBackfillIDs function

bc567a4

It has the same input as output of GetExpiredBackfillIDs function. Some backfillIDs can be acknowledged in between,But this should not affect the work of cleanup service.

Frontend: Add AcknowledgeBackfill method

9b5cfde

Used CreateBackfill implementation from another PR.

google-cla bot added the cla: yes label Nov 25, 2020

hsorellana reviewed Nov 25, 2020

View reviewed changes

internal/app/frontend/frontend_service.go Outdated Show resolved Hide resolved

hsorellana reviewed Nov 25, 2020

View reviewed changes

internal/app/frontend/frontend_service.go Show resolved Hide resolved

aLekSer added 2 commits December 8, 2020 22:12

Merge branch 'master' into frontend/acknowledge-backfill

7960e31

Applying comments

ec34b3b

Return back missing test after the merge.

aLekSer force-pushed the frontend/acknowledge-backfill branch from c9c664e to ec34b3b Compare December 8, 2020 19:22

aLekSer added 3 commits December 8, 2020 23:50

Add E2E test for AcknowledggeBackfill

0d84626

Refactored 2 tests, create match functionality reused

a52040b

Needed in 2 tests - AcknowledgeBackfill and ProposedBackfillCreate.

Add helper function to one more test TestProposedBackfillUpdate

4a86fd4

aLekSer marked this pull request as ready for review December 8, 2020 20:58

akremsa reviewed Dec 9, 2020

View reviewed changes

aLekSer force-pushed the frontend/acknowledge-backfill branch 2 times, most recently from c93f0b1 to 151e7a8 Compare December 9, 2020 11:16

Linter fixes

f43894c

test fixes.

aLekSer force-pushed the frontend/acknowledge-backfill branch from 151e7a8 to f43894c Compare December 9, 2020 11:50

akremsa suggested changes Dec 9, 2020

View reviewed changes

akremsa reviewed Dec 9, 2020

View reviewed changes

internal/app/frontend/frontend_service.go Show resolved Hide resolved

Return back Last Acknowledgment time update

7b4f7db

hsorellana reviewed Dec 9, 2020

View reviewed changes

internal/app/frontend/frontend_service.go Show resolved Hide resolved

aLekSer force-pushed the frontend/acknowledge-backfill branch from 051c741 to 7b4f7db Compare December 9, 2020 16:27

akremsa reviewed Dec 9, 2020

View reviewed changes

Fix nil input handling, add 2 UTs

c0b35d8

aLekSer force-pushed the frontend/acknowledge-backfill branch from 9f7440c to c0b35d8 Compare December 9, 2020 23:17

akremsa reviewed Dec 10, 2020

View reviewed changes

internal/app/frontend/frontend_service_test.go Outdated Show resolved Hide resolved

akremsa reviewed Dec 10, 2020

View reviewed changes

internal/app/frontend/frontend_service_test.go Show resolved Hide resolved

Fix timing issue

6c35005

When FetchMatches takes more than 200 ms pendingReleaseTimeout Ticket got expired and no Tickets can be queried thereafter.

Laremere reviewed Dec 10, 2020

View reviewed changes

Update Tickets associated with backfill, remove all assigned

58cc840

Add Mutex lock, UpdateBackfill accordingly after UpdateAssignments call.

thisisnotapril requested review from calebatwd and HazWard December 14, 2020 23:38

Merge branch 'master' into frontend/acknowledge-backfill

6474323

aLekSer requested a review from syntxerror as a code owner December 16, 2020 08:23

sawagh mentioned this pull request Dec 17, 2020

Clean up expired backfills #1297

Merged

davidedmonds approved these changes Dec 17, 2020

View reviewed changes

andrewgrundy approved these changes Dec 17, 2020

View reviewed changes

sawagh suggested changes Dec 18, 2020

View reviewed changes

aLekSer added 2 commits December 23, 2020 16:18

Rename statestore function AcknowledgeBackfill

aabc3ce

New function name seems more reasonable as it do only Redis timestamp updates, updateAcknowledgmentTimestamp.

Merge branch 'master' into frontend/acknowledge-backfill

7f76f6c

yeukovichd reviewed Dec 23, 2020

View reviewed changes

internal/statestore/backfill_test.go Outdated Show resolved Hide resolved

yeukovichd reviewed Dec 23, 2020

View reviewed changes

internal/statestore/backfill.go Outdated Show resolved Hide resolved

Apply comments

306856a

aLekSer requested a review from sawagh December 23, 2020 14:07

aLekSer added 3 commits December 23, 2020 17:19

Add Generation autoincrement test in test helper func

57854f4

Undo refactoring of createMatchWithBackfill

2422068

Undo unwanted changes to E2E

d99bb24

Add more logic as in doAssign() function

d68df51

We need to deindex tickets and add error logging for all NotFound tickets.

sawagh approved these changes Dec 26, 2020

View reviewed changes

aLekSer merged commit defac90 into googleforgames:master Dec 28, 2020

syntxerror added this to the v1.2.0 milestone Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frontend acknowledge backfill #1293

Frontend acknowledge backfill #1293

aLekSer commented Nov 25, 2020 •

edited

Loading

aLekSer commented Nov 26, 2020

akremsa Dec 9, 2020

aLekSer Dec 9, 2020

aLekSer Dec 9, 2020

akremsa Dec 9, 2020

akremsa Dec 9, 2020

aLekSer Dec 9, 2020

akremsa Dec 9, 2020

aLekSer Dec 9, 2020 •

edited

Loading

akremsa Dec 9, 2020 •

edited

Loading

aLekSer Dec 10, 2020

aLekSer commented Dec 9, 2020

Laremere Dec 10, 2020

Laremere Dec 10, 2020

aLekSer Dec 15, 2020

aLekSer Dec 16, 2020 •

edited

Loading

aLekSer commented Dec 11, 2020

thisisnotapril commented Dec 14, 2020

davidedmonds left a comment

sawagh Dec 17, 2020

aLekSer Dec 23, 2020

sawagh Dec 24, 2020

aLekSer Dec 24, 2020 •

edited

Loading

sawagh Dec 25, 2020

aLekSer Dec 28, 2020

sawagh Dec 17, 2020

aLekSer Dec 23, 2020

sawagh Dec 24, 2020

aLekSer Dec 24, 2020 •

edited

Loading

aLekSer commented Dec 23, 2020 •

edited

Loading

aLekSer commented Dec 23, 2020 •

edited

Loading

aLekSer commented Dec 23, 2020

aLekSer commented Dec 24, 2020

sawagh left a comment

Frontend acknowledge backfill #1293

Frontend acknowledge backfill #1293

Conversation

aLekSer commented Nov 25, 2020 • edited Loading

aLekSer commented Nov 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aLekSer Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

akremsa Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aLekSer commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aLekSer Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

aLekSer commented Dec 11, 2020

thisisnotapril commented Dec 14, 2020

davidedmonds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aLekSer Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aLekSer Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

aLekSer commented Dec 23, 2020 • edited Loading

aLekSer commented Dec 23, 2020 • edited Loading

aLekSer commented Dec 23, 2020

aLekSer commented Dec 24, 2020

sawagh left a comment

Choose a reason for hiding this comment

aLekSer commented Nov 25, 2020 •

edited

Loading

aLekSer Dec 9, 2020 •

edited

Loading

akremsa Dec 9, 2020 •

edited

Loading

aLekSer Dec 16, 2020 •

edited

Loading

aLekSer Dec 24, 2020 •

edited

Loading

aLekSer Dec 24, 2020 •

edited

Loading

aLekSer commented Dec 23, 2020 •

edited

Loading

aLekSer commented Dec 23, 2020 •

edited

Loading