Redis: Backfill last acknowledged #1288

aLekSer · 2020-11-19T13:23:09Z

What this PR does / Why we need it:
Add SortedSet to store Backfill in a sorted order to retrieve all expired Backfills.

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

Special notes for your reviewer:
This PR is dependent on StateStore changes.
We can select a specific TTL config variable later during implementation of Cleanup Service.

func (rb *redisBackend) AcknowledgeBackfill(ctx context.Context, id string) (error) {
func (rb *redisBackend) GetExpiredBackfills(ctx context.Context) ([]string, error) {
// Used in DeleteBackfil()
func (rb *redisBackend) deleteExpiredBackfillID(ctx context.Context, backfillID string) error

google-cla · 2020-11-19T13:23:24Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

hsorellana · 2020-11-19T18:45:23Z

Hey @aLekSer, look at this. "Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request"

google-cla · 2020-11-19T18:45:34Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

aLekSer · 2020-11-19T21:56:29Z

@hsorellana that would not be an issue once Alexey's PR would be merged. Then this PR would have only my changes.

Laremere · 2020-11-23T18:16:47Z

internal/statestore/backfill.go

+}
+
+// DeleteExpiredBackfillIDs - delete expired BackfillIDs from a sorted set
+func (rb *redisBackend) DeleteExpiredBackfillIDs(ctx context.Context, backfillIDs []string) error {


It's not clear to me how this will be used?

It would be used by a cleanup service:
1st step backfillIds, err := GetExpiredBackfills()
2nd - Return associated tickets for all Backfills back to the pool.
Some AcknowledgeBackfill() could occur in between
3rd step DeleteExpiredBackfillIDs(backfillIds), note that there could be more expired Backfills but we only delete thos e processed.

Why not just use the same mechanics as the delete method? Reduces implementation complexity and possible bugs.

Some DeleteBackfill() request can have some issues, others no, so Backend have an opportunity to select on its own.

Got an idea now. This can be done as part of DeleteBackfill

Laremere · 2020-11-23T18:18:39Z

internal/statestore/backfill.go

+	defer handleConnectionClose(&redisConn)
+
+	// the same TTL is used for both Backfills and pendingRelease Tickets
+	ttl := rb.cfg.GetDuration("pendingReleaseTimeout")


This should be something like 80% to 90% of ttl, since the backfill should expire before the tickets waiting on it, as the tickets timing out but the backfill NOT timing out but being acknowledged would be a problem.

akremsa · 2020-11-23T21:30:51Z

internal/statestore/backfill.go

+	}
+	defer handleConnectionClose(&redisConn)
+
+	currentTime := time.Now().UnixNano()


@Laremere is there an agreement of not using UTC()? time.Now() is used everywhere.

Assumption is all computers in a cluster will have clocks that are synchronized enough.

akremsa · 2020-11-23T21:33:39Z

internal/statestore/backfill.go

+	}
+	defer handleConnectionClose(&redisConn)
+
+	cmds := make([]interface{}, 0, len(backfillIDs)+1)


akremsa · 2020-11-23T21:55:21Z

internal/statestore/backfill_test.go

@@ -301,3 +302,54 @@ func TestDeleteBackfill(t *testing.T) {
 	require.Equal(t, codes.Unavailable.String(), status.Convert(err).Code().String())
 	require.Contains(t, status.Convert(err).Message(), "DeleteBackfill, id: 12345, failed to connect to redis:")
 }
+
+func TestAcknowledgeBackfill(t *testing.T) {


Actually, it's not an AcknowledgeBackfill unit test now. You test here the whole acknowledge scenario. This is nice, but I would call this test something like TestAcknowledgeLifecycle.
To write a unit test you need to test exactly one single unit (method) under the different conditions.
Talking about backfill, it's enough to check that after calling AcknowledgeBackfill the passed ID is added to backfill_last_ack_time sorted set.
So I suggest you to rename this method and add TestAcknowledgeBackfill where you will test only AcknowledgeBackfill method. What do you think?

akremsa · 2020-11-23T21:57:37Z

internal/statestore/public.go

@@ -84,6 +84,15 @@ type Service interface {

 	// NewMutex returns an interface of a new distributed mutex with given name
 	NewMutex(key string) RedisLocker
+
+	// AcknowledgeBackfill - store Backfill's last accessed time
+	AcknowledgeBackfill(ctx context.Context, id string) error


nit: I suggest sticking to the current comments format like:
AcknowledgeBackfill stores Backfill's last accessed time

// AcknowledgeBackfill - stores Backfill's last acknowledgement time

akremsa · 2020-11-23T22:01:36Z

internal/statestore/backfill.go

+	_, err = redisConn.Do("ZADD", cmds...)
+	if err != nil {
+		err = errors.Wrap(err, "failed to store backfill's last acknowledgement time")
+		return status.Error(codes.Internal, err.Error())


nit: I think here you can use status.Errorf(...., err) as you do above. Probably we need to elaborate the same error logging format across this file.

akremsa · 2020-11-25T12:24:00Z

internal/statestore/backfill.go

+	endTimeInt := curTime.Add(-ttl).UnixNano()
+	startTimeInt := 0
+
+	// Filter out tickets that are fetched but not assigned within TTL time (ms).


Filter out tickets backfill ids ...

aLekSer · 2020-11-25T22:15:09Z

@Laremere Scott, note that I would add config parameter for Backfill Time in a separate PR to reduce complexity of this PR. It is ready to review, hope that I got all comments right.

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.

Laremere · 2020-11-26T09:13:10Z

Force push is adding more work to re-review. Friendly reminder to not do that (otherwise "see changes since your last view" breaks.)
If you're having trouble pushing (eg, you used update branch button at some point), then do git pull before you push.

aLekSer · 2020-11-26T09:32:53Z

@Laremere Do you mean that we should not perform rebase on a new master also? I am used to do it on Agones.

Read and update Redis content to make them independent of other functions.

akremsa · 2020-11-27T14:23:58Z

internal/statestore/backfill.go

+}
+
+// deleteExpiredBackfillID - delete expired BackfillID from a sorted set
+func (rb *redisBackend) deleteExpiredBackfillID(ctx context.Context, backfillID string) error {


you can pass redis connection here from a caller method. what do you think?

akremsa · 2020-11-27T16:06:59Z

internal/statestore/backfill.go

+	_, err := conn.Do("ZREM", cmds...)
+	if err != nil {
+		return status.Errorf(codes.Internal, "failed to delete expired backfill ID %v",
+			errors.Wrap(err, "failed to delete expired backfill ID from Sorted Set"))


You use the same two error messages here, please clean it up.

akremsa · 2020-11-27T16:12:18Z

internal/statestore/backfill_test.go

 			}
+
+			// test that Backfill also deleted from last acknowledged sorted set
+			_, err = redis.Int64(conn.Do("ZSCORE", backfillLastAckTime, tc.backfillID))


there is no point to perform this check if you are testing the case with an empty backfill id. I think it's better to put it under if tc.backfillID != "" statement.

Laremere · 2020-11-30T06:35:21Z

@Laremere Do you mean that we should not perform rebase on a new master also? I am used to do it on Agones.

We squash merge all PRs anyways, so rebasing isn't necessary.

akremsa · 2020-11-30T10:13:51Z

internal/statestore/backfill_test.go

+				// test that Backfill last acknowledged is in a sorted set
+				ts, err := redis.Int64(conn.Do("ZSCORE", backfillLastAckTime, tc.backfillID))
+				require.NoError(t, err)
+				require.True(t, ts > 0, "timestamp is valid")


timestamp is NOT valid

Fixing error returned and text in Test failure.

aLekSer · 2020-11-30T14:42:53Z

internal/statestore/backfill.go

+	}
+	defer handleConnectionClose(&redisConn)
+
+	// Use a fraction 80% of pendingRelease Tickets TTL


Will update this in this or the next PR - Create a new config parameter.

Laremere · 2020-11-30T20:47:58Z

internal/statestore/backfill.go

+	cmds := make([]interface{}, 0)
+	cmds = append(cmds, backfillLastAckTime, currentTime, id)
+
+	_, err = redisConn.Do("ZADD", cmds...)


There's no need for the oddness with cmds, just do:
_, err = redisConn.Do("ZADD", backfillLastAckTime, currentTime, id)

Laremere · 2020-11-30T20:49:06Z

internal/statestore/backfill.go

@@ -28,7 +29,8 @@ import (
 )

 const (
-	allBackfills = "allBackfills"
+	backfillLastAckTime = "backfill_last_ack_time"


This also needs to be set during backfill creation, so that a server which never starts up can be cleaned up.

Yes, that's true, made a TODO for this I wanted to make this as part of separate PR.
But I think can be added here also.

Added additional call acknowledgeBackfill to StateStore CreateBackfill.

Laremere · 2020-11-30T20:50:16Z

internal/statestore/backfill.go

+	cmds := make([]interface{}, 0, 2)
+	cmds = append(cmds, backfillLastAckTime, backfillID)
+
+	_, err := conn.Do("ZREM", cmds...)


Laremere · 2020-11-30T20:54:38Z

internal/statestore/backfill_test.go

+	require.NoError(t, err)
+	err = service.AcknowledgeBackfill(ctx, bf2)
+	require.NoError(t, err)
+	// Sleep until the pending release expired and verify we still have all the tickets


After acknowledging, but before sleeping, verify the expired backfills.

Fixed tests and Redis commands.

The better place to do updates on CreateBackfill. Reuse one connection, added a helper function for that.

akremsa · 2020-12-01T13:40:22Z

internal/statestore/backfill.go

@@ -59,6 +61,8 @@ func (rb *redisBackend) CreateBackfill(ctx context.Context, backfill *pb.Backfil
 	if res.(int64) == 0 {
 		return status.Errorf(codes.AlreadyExists, "backfill already exists, id: %s", backfill.GetId())
 	}
+
+	acknowledgeBackfill(redisConn, backfill.GetId())


you are ignoring an error here

yes, thanks for noticing, will update

TestCreateBackfill should be updated according to this new behavior.

Don't CreateBackfill prior to running AckBackfill it is covered in CreateBackfill.

aLekSer · 2020-12-01T21:59:39Z

@Laremere all comments got resolved.

google-cla bot added the cla: no label Nov 19, 2020

aLekSer force-pushed the redis/backfill-last-ack branch from 967f4d7 to 8aa7ad8 Compare November 23, 2020 08:42

google-cla bot added cla: yes and removed cla: no labels Nov 23, 2020

aLekSer force-pushed the redis/backfill-last-ack branch from 8aa7ad8 to bc567a4 Compare November 23, 2020 08:52

aLekSer marked this pull request as ready for review November 23, 2020 10:23

Laremere reviewed Nov 23, 2020

View reviewed changes

akremsa reviewed Nov 23, 2020

View reviewed changes

akremsa reviewed Nov 25, 2020

View reviewed changes

aLekSer changed the title ~~Redis/backfill last ack~~ Redis: backfill last acknowledged Nov 25, 2020

aLekSer mentioned this pull request Nov 25, 2020

Frontend acknowledge backfill #1293

Merged

aLekSer changed the title ~~Redis: backfill last acknowledged~~ Redis: Backfill last acknowledged Nov 25, 2020

aLekSer requested a review from Laremere November 25, 2020 21:41

aLekSer added 3 commits November 26, 2020 01:15

StateStore: add LastAcknowledge timestamp to Backfill

e306f34

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

Add DeleteExpiredBackfillIDs function

c7013cf

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.

Applied comments - made deleteExpiredBackfillID private

fbc7ff6

aLekSer force-pushed the redis/backfill-last-ack branch from b722c97 to fbc7ff6 Compare November 25, 2020 22:15

Linter fix

b651247

Add unit tests for AcknowledgeBackfill and GetExpiredBackillIds

306cc8b

Read and update Redis content to make them independent of other functions.

akremsa reviewed Nov 27, 2020

View reviewed changes

aLekSer added 3 commits November 27, 2020 18:01

Applied comments from PR

104607f

Merge branch 'master' into redis/backfill-last-ack

87f457a

Fix nit - func comment

fbd4950

akremsa reviewed Nov 27, 2020

View reviewed changes

akremsa approved these changes Nov 27, 2020

View reviewed changes

aLekSer added 2 commits November 30, 2020 10:42

Merge branch 'master' into redis/backfill-last-ack

1114101

Update comments and UT

c108334

akremsa reviewed Nov 30, 2020

View reviewed changes

Applying comments

5873bc3

Fixing error returned and text in Test failure.

aLekSer commented Nov 30, 2020

View reviewed changes

Merge branch 'master' into redis/backfill-last-ack

489073f

Laremere suggested changes Nov 30, 2020

View reviewed changes

aLekSer added 2 commits December 1, 2020 16:25

Add AcknowledgeBackfill call on creating Backfills

748141c

Fixed tests and Redis commands.

Moved acknowledgeBackfill function from FE to statestore

fb95bf8

The better place to do updates on CreateBackfill. Reuse one connection, added a helper function for that.

akremsa reviewed Dec 1, 2020

View reviewed changes

aLekSer added 3 commits December 1, 2020 16:57

Fixing tests and comments

2f25a01

Simplify test a bit for AcknowledgeBackfill

7b15b83

Don't CreateBackfill prior to running AckBackfill it is covered in CreateBackfill.

Fix one more test

a494c69

aLekSer force-pushed the redis/backfill-last-ack branch from af6645c to a494c69 Compare December 1, 2020 20:54

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

Laremere approved these changes Dec 2, 2020

View reviewed changes

Laremere merged commit 26d1aa2 into googleforgames:master Dec 2, 2020

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

syntxerror added breaking api change Breaking API change that may require migration for existing customers to update to new version. area/feature labels Mar 29, 2021

syntxerror removed the breaking api change Breaking API change that may require migration for existing customers to update to new version. label Apr 15, 2021

Redis: Backfill last acknowledged #1288

Redis: Backfill last acknowledged #1288

Conversation

aLekSer commented Nov 19, 2020 • edited

google-cla bot commented Nov 19, 2020

hsorellana commented Nov 19, 2020

google-cla bot commented Nov 19, 2020

aLekSer commented Nov 19, 2020

Choose a reason for hiding this comment

aLekSer Nov 23, 2020 • edited

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

Choose a reason for hiding this comment

akremsa Nov 23, 2020 • edited

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 Nov 25, 2020

Laremere commented Nov 26, 2020

aLekSer commented Nov 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laremere commented Nov 30, 2020

Choose a reason for hiding this comment

aLekSer Nov 30, 2020 • edited

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

akremsa Dec 1, 2020 • edited

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

aLekSer commented Nov 19, 2020 •

edited

aLekSer Nov 23, 2020 •

edited

akremsa Nov 23, 2020 •

edited

aLekSer commented Nov 26, 2020 •

edited

aLekSer Nov 30, 2020 •

edited

akremsa Dec 1, 2020 •

edited