Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Backfill support to internal statestore #1273

Merged
merged 15 commits into from
Nov 19, 2020

Conversation

akremsa
Copy link

@akremsa akremsa commented Oct 28, 2020

What this PR does / Why we need it:
Added Backfill support to internal statestore.
Methods: create, get, delete, update.
Renamed redis.go/redis_test.go to ticket.go/ticket_test.go because now there are 2 entities - ticket and backfill and I find such naming to be more consistent.

@akremsa akremsa changed the title Added Backfill support to internal statestore Add Backfill support to internal statestore Oct 28, 2020
@akremsa akremsa force-pushed the backfill-redis branch 3 times, most recently from 2e74094 to 68fe81e Compare October 29, 2020 10:50
api/messages.proto Outdated Show resolved Hide resolved
internal/statestore/backfill.go Show resolved Hide resolved
internal/statestore/backfill.go Outdated Show resolved Hide resolved
@akremsa akremsa force-pushed the backfill-redis branch 2 times, most recently from 9090646 to 4e27552 Compare November 5, 2020 14:23
@akremsa
Copy link
Author

akremsa commented Nov 6, 2020

@Laremere
There are 2 ways how we can update storing BF and ticketIDs.

  1. Use a single key for BF and associated ticketIDs. In this case, ticketIDs can be added either to backfill protobuf or just use an internal structure like:
type RedisBackfill struct {
backfill Backfill
ticketIDs []string
}

In this case, we can keep using transactions (WATCH, MULTI) because we have only 1 instance of backfill entity with all information inside.
Side effect: there is an error on an UpdateBackfill step, but tickets have been put to PendingRelease state. We have separate keys for backfill and pending_tickets.
Do we need to perform these 2 operations - Update and Pending Release in a scope of a single transaction?
If no - then we can use this approach.

  1. Use 2 keys: backfillID for backfill, backfillID-tickets for backfill-tickets mapping. In this case, we can use only distributed locks. But we do not have an opportunity to roll back using only locks!

We need to choose an approach we are going to follow. I can make an update in the upcoming PR.

@Laremere
Copy link

Laremere commented Nov 9, 2020

The lock approach seems more initially trackable - if we're not locking we need to be very sure there aren't race conditions.

If we want to store items in redis that aren't a public proto, that's fine - there's internal protobufs too, which can be used.

Need to not only consider the backfills here - when a backfill is acknowledged, the tickets which are acknowledged need to be assigned and then removed from the backfill's pending tickets.

api/messages.proto Outdated Show resolved Hide resolved
internal/statestore/backfill.go Outdated Show resolved Hide resolved
internal/statestore/backfill.go Outdated Show resolved Hide resolved
internal/statestore/redis.go Show resolved Hide resolved
@akremsa akremsa force-pushed the backfill-redis branch 2 times, most recently from fa182e4 to 2986ca2 Compare November 12, 2020 21:30
@akremsa
Copy link
Author

akremsa commented Nov 12, 2020

@Laremere updated PR, please check.

  1. I decided to store backfill and ticketIDs together. In this way, we avoid a case when ticketIDs are set successfully, but the next call to set backfill fails (or vice versa) and there is no way to rollback because we agreed to use locks instead of transactions.
  2. Added an additional internal protobuf structure to store backfill and ticketIDs.
  3. Mutex is returned to the caller because otherwise it should be stored inside statestore between lock and unlock calls which I've found a bit problematic.

There should be the following flow using statestore:

statestore := New(cfg)
mutex := statestore.NewMutex("key1")
mutex.Lock()
statestore.ReleaseAllTickets(ctx)
statestore.UpdateBackfill(ctx, b, tids)
mutex.Unlock()

internal/api/internal.proto Outdated Show resolved Hide resolved
internal/statestore/public.go Outdated Show resolved Hide resolved
})
}

// pass an expired context, err expected

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a different test, it belongs in its own test function.

Here, and for the other backfill functions.

Copy link
Author

@akremsa akremsa Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Laremere Why? I use high-level names for tests where I include multiple test cases.
For instance - TestCreateBackfill means that I test CreateBackfill under different conditions, and passing an expired context can be considered as another test case.

Exactly the same happens here:
https://github.com/googleforgames/open-match/blob/master/internal/app/frontend/frontend_service_test.go#L45

Expire context scenario is included in a general TestDoCreateTickets function. In my case, I didn't add preAction, but tested expired context case at the end of the function.

If you want to extract expired context test case to its own function it should be done in all tests, not only in backfill_test.go, which can take some time I guess. What do you think?

@aLekSer
Copy link
Collaborator

aLekSer commented Nov 17, 2020

redis.go file is big, so my comments got hidden:
Comment to this line:

func (r redisLocker) Lock() error {

I faced an issue while using this call in my development branch and test TestDoDeleteBackfill, test case "expect unavailable code since context is canceled before being called" which is a replica of existent ticket version.
It is failed while waiting for Unavailable error, when run Lock without a context:
https://github.com/aLekSer/open-match/blob/c6c7ecc702d3539f1f60d7413a75833577cc956c/internal/app/frontend/frontend_service_test.go#L319
Suggested version of Lock function:


// Lock locks r. In case it returns an error on failure, you may retry to acquire the lock by calling this method again.
func (r redisLocker) Lock(ctx context.Context) error {
	err := r.mutex.LockContext(ctx)
	if err != nil {
		return status.Errorf(codes.Unavailable, "Lock, failed to connect to redis: %v", err)
	}
	return err
}

Attaching commit link for easy navigation.

@syntxerror syntxerror added this to the v1.2.0 milestone Nov 18, 2020
Copy link
Collaborator

@aLekSer aLekSer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for applying all comments.

@Laremere Laremere merged commit e2247a7 into googleforgames:master Nov 19, 2020
@syntxerror syntxerror added area/feature breaking api change Breaking API change that may require migration for existing customers to update to new version. labels Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/feature breaking api change Breaking API change that may require migration for existing customers to update to new version. cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants