Frontend: UpdateBackfill and DeleteBackfill handlers #1292

aLekSer · 2020-11-25T13:14:54Z

What this PR does / Why we need it:

UD methods for Backfill structure.

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

Special notes for your reviewer:
This PR contains another PR with CRud part of Frontend: #1279

So review please only changes from this commit and later commits:
9f970a7

internal/app/frontend/frontend_service.go

yeukovichd · 2020-11-25T15:58:00Z

internal/app/frontend/frontend_service.go

+	if req.Backfill == nil {
+		return nil, status.Errorf(codes.InvalidArgument, ".backfill is required")
+	}
+	if req.Backfill.CreateTime != nil {


Why is this check required? If we look at doCreateBackfill it always assigns new value to CreateTime.

This is a leftover of CreateTicket and is part of #1279 This PR would be rebased on top of master after #1279 would be merged.

yeukovichd · 2020-11-25T16:17:55Z

internal/app/frontend/frontend_service.go

+		if _, err = m.Unlock(ctx); err != nil {
+
+			logger.WithFields(logrus.Fields{
+				"error": err.Error(),


Maybe it is better to use WithError and specify fields such as backfill_id?

aLekSer · 2020-11-26T00:25:57Z

Unrelated to the change CI failure:


time="2020-11-26T00:14:25Z" level=error msg="context canceled" app=openmatch component=api.test
--- FAIL: TestMatchFunctionMatchCollision (0.43s)
    fetch_matches_test.go:158: 
        	Error Trace:	fetch_matches_test.go:158
        	Error:      	Should be true
        	Test:       	TestMatchFunctionMatchCollision
        	Messages:   	381.29587ms

Generate ID, store into Redis, add telemetry.

Add lock/unlock to Frontend Create method.

Create and GetBackfill E2E test.

Add CreateBackfill validation check. Extend E2E test.

We need to create a new statestore, so cancel() would always succeed.

New frontend methods with locks and tests.

Fixing error handling adding UT similar to CreateBackfill.

Laremere · 2020-11-30T06:26:14Z

internal/app/frontend/frontend_service.go

+		return nil, err
+	}
+
+	err = s.store.DeleteTicketsFromPendingRelease(ctx, associatedTickets)


This must be done after the ticket is updated:
If the server performs this action, then dies, the tickets will think they can return to the active pool and find other matches, but the backfill will still think it can put tickets in this backfill, potentially assigning them twice.

The other way around, the backfill will release it's claim in the tickets, and if this server dies, then the tickets will just time out and then re-enter the pool.

Laremere · 2020-11-30T06:29:16Z

internal/app/frontend/frontend_service.go

+		return nil, status.Errorf(codes.InvalidArgument, ".backfill is required")
+	}
+
+	backfill, ok := proto.Clone(req.Backfill).(*pb.Backfill)


The update here should be done defensively, currently you perform the get, but use the backfill passed in as the source of truth. Instead start with the backfill from the get, and then update each field which should be updated. Eg, create time would be overwritten if the wrong value were sent here.

Updated the code with:

// Update generation here, because Frontend is used by GameServer only bfStored.Generation++ bfStored.SearchFields = backfill.SearchFields bfStored.Extensions = backfill.Extensions

Laremere · 2020-11-30T06:30:41Z

internal/testing/e2e/backfill_test.go

+	require.Nil(t, err)
+
+	createdBf.SearchFields.StringArgs["key"] = "val"
+	updatedBf, err := om.Frontend().UpdateBackfill(ctx, &pb.UpdateBackfillRequest{Backfill: createdBf})


To better test this method, send an entirely new Backfill protobuf object, with the new value but setting the ID to the one from createdBf.

As it is, this test incorrectly thinks it verifies CreateTime can't be updated.

Also it would be a better way to show that generation doesn't need to be right.

Laremere · 2020-11-30T06:31:34Z

internal/testing/e2e/backfill_test.go

+		},
+	},
+	}
+	createdBf, err := om.Frontend().CreateBackfill(ctx, &pb.CreateBackfillRequest{Backfill: bf})


Test generation values?

Generation, CreateTime is ignored on Backfill updates, fixing test to check that.

aLekSer · 2020-12-01T22:00:05Z

All comments are resolved.

Laremere · 2020-12-02T05:10:56Z

/gcprun

Laremere · 2020-12-02T05:26:30Z

/gcbrun

Laremere

Extra comment should be removed, but a few things are depending on this so I'll merge it if the tests pass.

internal/app/frontend/frontend_service.go

…h into frontend/update-del-bf

Laremere · 2020-12-02T19:27:43Z

Tests are still failing....

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

aLekSer force-pushed the frontend/update-del-bf branch from fdbeaaa to 9f970a7 Compare November 25, 2020 14:41

yeukovichd reviewed Nov 25, 2020

View reviewed changes

aLekSer force-pushed the frontend/update-del-bf branch from c19f681 to c60a4f6 Compare November 26, 2020 00:08

aLekSer added 10 commits November 26, 2020 10:01

Frontend: Add createBackfill method

9b53ba4

Generate ID, store into Redis, add telemetry.

StateStore interface updates

f4a05c9

Add lock/unlock to Frontend Create method.

Add GetBackfill frontend function, E2E test

481dbcc

Create and GetBackfill E2E test.

Applying comments

ca91366

Add CreateBackfill validation check. Extend E2E test.

Switch test to table version, merged two tests into one

39c34c9

Remove cancel() func test, as it is asynchronous

01e66b1

Add back cancel

426b368

We need to create a new statestore, so cancel() would always succeed.

Frontend: Update and Delete backfill methods

fd7779d

New frontend methods with locks and tests.

More updates to UpdateBackfill function

bfb323f

Fixing error handling adding UT similar to CreateBackfill.

Leftover after merging master

1252491

aLekSer force-pushed the frontend/update-del-bf branch from c60a4f6 to 1252491 Compare November 26, 2020 07:09

Laremere suggested changes Nov 30, 2020

View reviewed changes

aLekSer added 2 commits December 2, 2020 00:17

Merge branch 'master' into frontend/update-del-bf

2f006fc

Add Index, DeindexBackfill, update only part of Backfill

cd57b15

Generation, CreateTime is ignored on Backfill updates, fixing test to check that.

aLekSer requested a review from Laremere December 1, 2020 21:58

Laremere approved these changes Dec 2, 2020

View reviewed changes

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

Scott Redig and others added 3 commits December 1, 2020 22:05

Merge branch 'master' into frontend/update-del-bf

156509f

Remove redundant comment

83d3e54

Merge branch 'frontend/update-del-bf' of github.com:aLekSer/open-matc…

037fb64

…h into frontend/update-del-bf

Merge branch 'master' into frontend/update-del-bf

4f0b5c1

aLekSer force-pushed the frontend/update-del-bf branch from 99bd1b7 to 4f0b5c1 Compare December 3, 2020 12:57

Fix test require.Equal(t, nil, something)

b314d4b

Laremere merged commit c85af44 into googleforgames:master Dec 3, 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: UpdateBackfill and DeleteBackfill handlers #1292

Frontend: UpdateBackfill and DeleteBackfill handlers #1292

aLekSer commented Nov 25, 2020 •

edited

Loading

yeukovichd Nov 25, 2020

aLekSer Nov 25, 2020

yeukovichd Nov 25, 2020

aLekSer commented Nov 26, 2020

Laremere Nov 30, 2020

Laremere Nov 30, 2020

aLekSer Dec 1, 2020

Laremere Nov 30, 2020

Laremere Nov 30, 2020

Laremere Nov 30, 2020

aLekSer commented Dec 1, 2020

Laremere commented Dec 2, 2020

Laremere commented Dec 2, 2020

Laremere left a comment

Laremere commented Dec 2, 2020

Frontend: UpdateBackfill and DeleteBackfill handlers #1292

Frontend: UpdateBackfill and DeleteBackfill handlers #1292

Conversation

aLekSer commented Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

aLekSer commented Dec 1, 2020

Laremere commented Dec 2, 2020

Laremere commented Dec 2, 2020

Laremere left a comment

Choose a reason for hiding this comment

Laremere commented Dec 2, 2020

aLekSer commented Nov 25, 2020 •

edited

Loading