Frontend Create Backfill #1279

aLekSer · 2020-11-09T13:01:30Z

Initial version of CreateBackfill. It stores an empty regular TicketIds list.

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:
Wait on StateStare changes.

google-cla · 2020-11-09T13:01:41Z

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-09T14:59:42Z

internal/app/frontend/frontend_service.go

+	err := store.CreateBackfill(ctx, backfill, []string{})
+	if err != nil {
+		return nil, err
+


There's a missing closing curly brace here

That's true, I cannot run the test without state store changes.

google-cla · 2020-11-10T13:59:16Z

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.

google-cla · 2020-11-12T13:51:49Z

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.

google-cla · 2020-11-13T12:30:10Z

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.

scosgrave · 2020-11-17T18:56:45Z

internal/api/internal.proto

@@ -0,0 +1,29 @@
+// Copyright 2020 Google LLC


I think statestore.proto might be a more appropriate name for this file. It matches the style of using the module name in the proto file name that synchronizer.proto has started.

messages.proto would also be appropriate, as we put all message definitions that aren't request/response types in a similarly named file in the public api.

Oh that file is actually being added by #1273

My initial take on how this file should be named was not right. Laramere educated me on how these files are named. The naming pattern is not based on the module name. Instead, services and their req/resp usually go into their own proto file, and objects that aren't coupled to a service's API usually go in a messages.proto file.
+1 for messages.proto

I will copy a following thread back to the author @akremsa

internal/app/frontend/frontend_service.go

akremsa · 2020-11-23T12:32:13Z

internal/testing/e2e/backfill_test.go

+	require.Nil(t, err)
+
+	// Different Ids should be generated
+	assert.NotEqual(t, b1.Id, b2.Id)


why do you mix assert and require?
please use require

akremsa · 2020-11-23T12:33:52Z

internal/testing/e2e/backfill_test.go

+	// Other than CreateTune abd Id fields, they should be equal
+	assert.Equal(t, b1, b2)
+	require.Nil(t, err)
+	get, err := om.Frontend().GetBackfill(ctx, &pb.GetBackfillRequest{BackfillId: b1.Id})


please use common naming like result or actual, it will be easier to read this code.

internal/testing/e2e/backfill_test.go

akremsa · 2020-11-23T12:38:15Z

internal/testing/e2e/backfill_test.go

+	assert.NotEqual(t, b1.Id, b2.Id)
+	b1.Id = b2.Id
+	b1.CreateTime = b2.CreateTime
+	// Other than CreateTune abd Id fields, they should be equal


What does it mean? Could you rephrase it, please?

I have updated this comment, but in a separate branch, sorry. I meant that all fields other than CreateTime and Ids are the same if we CreateBackfill with the same input args.

internal/app/frontend/frontend_service.go

akremsa · 2020-11-23T12:40:30Z

internal/app/frontend/frontend_service_test.go

+			ctx, cancel := context.WithCancel(utilTesting.NewContext(t))
+			test.preAction(cancel)
+
+			res, err := doCreateBackfill(ctx, &pb.CreateBackfillRequest{Backfill: test.backfill}, store)


require.NotNil(t, res)

Why do you test doCreateBackfill instead of CreateBackfill?

I am testing real handlers as part of E2E test.

This is a unit test, it has nothing to do with e2e one. doCreateBackfill is not an entry point to the current package. Please, test exported methods only, as you do in TestDoGetBackfill. Otherwise, you just skip all code which comes before doCreateBackfill.

First of all we follow the same pattern for TestDoCreateTicket(), second Scott asked me to use E2E to cover those handlers:
https://github.com/googleforgames/open-match/pull/1279/files#r528961563

You should not rely on any other tests (e2e, etc) while working on unit tests. Here you ignore everything which comes before doCreateBackfill.

@Laremere do you mean that we should test public API in e2e tests, rather than unit ones?
Because at this moment I don't see a clear boundary between tickets e2e and tickets unit tests, for instance.
If so - there is no point to add and support tests in frontend package right now.

With TestDoCreateTickets half of the method is tested in frontend package, another part (validations) in e2e, which is a bit confusing.

akremsa

Some minor changes requested

internal/app/frontend/frontend_service.go

Laremere · 2020-11-23T19:59:16Z

internal/app/frontend/frontend_service_test.go

@@ -88,6 +88,62 @@ func TestDoCreateTickets(t *testing.T) {
 	}
 }

+func TestDoCreateBackfill(t *testing.T) {


While I think this follows the patterns of the testing for the other calls, I'm unsure about the value these tests are adding:

I don't think calling with canceled context actually finds problems, at most if checks that one use of context is done correctly, but not that all uses are correct.

It took a bit to get there, but I'm pretty confident that "preAction" is an anti-pattern at this point. It's adding too much logic to tests.

See also, the if err != nil check in the test code is definitely logic. This should be removed, and is evidence of the tests being over-generalized.

My recommendation:
Get the test coverage using the e2e tests for:

creating

getting backfill created

getting backfill which isn't present, and getting not found
(first two are already done, as far as I can tell)

At most, keep only the canceled context tests here, but I'm fine not having those.
The other tests that follow these patterns for existing features should probably be cleaned up, but that's out of scope for this project.

Thanks for your feedback, will address all comments

Generate ID, store into Redis, add telemetry.

Add lock/unlock to Frontend Create method.

Create and GetBackfill E2E test.

for E2E tests.

Add CreateBackfill validation check. Extend E2E test.

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

aLekSer · 2020-11-26T06:03:15Z

Hello @Laremere I have applied all comments, if I did something wrong please leave feedback.

Laremere

Adding a few comments, don't want to block this on nit for testing, so merging with comments. Cleanup to match my desired state doesn't involve only the new tests here, so it's fine.

Laremere · 2020-11-26T06:53:15Z

internal/app/frontend/frontend_service_test.go

+		tc := tc
+		t.Run(tc.description, func(t *testing.T) {
+			res, err := fs.CreateBackfill(ctx, tc.request)
+			if tc.expectedCode == codes.OK {


Avoid clauses like this in tests: instead this should be two separate test cases.

Laremere · 2020-11-26T06:53:28Z

internal/app/frontend/frontend_service_test.go

+		})
+	}
+
+	// expect error with canceled context


Should be in a separate test case.

Laremere · 2020-11-26T06:53:58Z

internal/testing/e2e/backfill_test.go

+	require.Equal(t, b1, actual)
+
+	// Get Backfill which is not present - NotFound
+	actual, err = om.Frontend().GetBackfill(ctx, &pb.GetBackfillRequest{BackfillId: b1.Id + "new"})


I think this should be a separate test case?

OK, I will make it as a separate PR.

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

aLekSer changed the title ~~Frontend create bf~~ Frontend create Backfill Nov 9, 2020

hsorellana reviewed Nov 9, 2020

View reviewed changes

aLekSer force-pushed the frontend-create-bf branch from 8aa0341 to 2577af9 Compare November 12, 2020 13:51

aLekSer force-pushed the frontend-create-bf branch from 2577af9 to 40ca365 Compare November 13, 2020 12:29

scosgrave reviewed Nov 17, 2020

View reviewed changes

hsorellana reviewed Nov 17, 2020

View reviewed changes

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

aLekSer mentioned this pull request Nov 17, 2020

Add Backfill support to internal statestore #1273

Merged

syntxerror added this to the v1.2.0 milestone Nov 18, 2020

aLekSer force-pushed the frontend-create-bf branch from 40ca365 to a5585a4 Compare November 23, 2020 08:10

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

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

akremsa reviewed Nov 23, 2020

View reviewed changes

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

akremsa reviewed Nov 23, 2020

View reviewed changes

internal/testing/e2e/backfill_test.go Show resolved Hide resolved

akremsa reviewed Nov 23, 2020

View reviewed changes

internal/testing/e2e/backfill_test.go Show resolved Hide resolved

akremsa reviewed Nov 23, 2020

View reviewed changes

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

akremsa reviewed Nov 23, 2020

View reviewed changes

akremsa suggested changes Nov 23, 2020

View reviewed changes

hsorellana reviewed Nov 23, 2020

View reviewed changes

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

Laremere reviewed Nov 23, 2020

View reviewed changes

akremsa approved these changes Nov 24, 2020

View reviewed changes

Frontend: Add createBackfill method

d489f74

Generate ID, store into Redis, add telemetry.

aLekSer added 7 commits November 25, 2020 11:27

StateStore interface updates

c292756

Add lock/unlock to Frontend Create method.

Add GetBackfill frontend function, E2E test

d37dbd9

Create and GetBackfill E2E test.

Linter fixes

d3adc6d

for E2E tests.

Applying comments

901d0b5

Add CreateBackfill validation check. Extend E2E test.

Switch test to table version, merged two tests into one

984d5af

Remove cancel() func test, as it is asynchronous

f65bde3

Add back cancel

471b617

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

aLekSer force-pushed the frontend-create-bf branch from 00975e6 to 471b617 Compare November 25, 2020 08:27

This was referenced Nov 25, 2020

Frontend: UpdateBackfill and DeleteBackfill handlers #1292

Merged

Frontend acknowledge backfill #1293

Merged

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

aLekSer changed the title ~~Frontend create Backfill~~ Frontend Create Backfill Nov 26, 2020

Laremere approved these changes Nov 26, 2020

View reviewed changes

Laremere merged commit 248494c into googleforgames:master Nov 26, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frontend Create Backfill #1279

Frontend Create Backfill #1279

aLekSer commented Nov 9, 2020

google-cla bot commented Nov 9, 2020

hsorellana Nov 9, 2020

aLekSer Nov 10, 2020

google-cla bot commented Nov 10, 2020

google-cla bot commented Nov 12, 2020

google-cla bot commented Nov 13, 2020

scosgrave Nov 17, 2020

Laremere Nov 17, 2020

Laremere Nov 17, 2020

scosgrave Nov 17, 2020

aLekSer Nov 17, 2020

akremsa Nov 23, 2020

akremsa Nov 23, 2020

akremsa Nov 23, 2020

aLekSer Nov 23, 2020 •

edited

akremsa Nov 23, 2020

akremsa Nov 23, 2020

aLekSer Nov 23, 2020

akremsa Nov 23, 2020 •

edited

aLekSer Nov 24, 2020

akremsa Nov 24, 2020 •

edited

akremsa Nov 24, 2020

akremsa Nov 24, 2020 •

edited

akremsa left a comment

Laremere Nov 23, 2020

aLekSer Nov 23, 2020

aLekSer commented Nov 26, 2020

Laremere left a comment

Laremere Nov 26, 2020

Laremere Nov 26, 2020

Laremere Nov 26, 2020

aLekSer Nov 26, 2020

Frontend Create Backfill #1279

Frontend Create Backfill #1279

Conversation

aLekSer commented Nov 9, 2020

google-cla bot commented Nov 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Nov 10, 2020

google-cla bot commented Nov 12, 2020

google-cla bot commented Nov 13, 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

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

akremsa Nov 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akremsa Nov 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akremsa Nov 24, 2020 • edited

Choose a reason for hiding this comment

akremsa left a 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 26, 2020

Laremere 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

Choose a reason for hiding this comment

aLekSer Nov 23, 2020 •

edited

akremsa Nov 23, 2020 •

edited

akremsa Nov 24, 2020 •

edited

akremsa Nov 24, 2020 •

edited