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

feat: get multi records by keys #410

Merged
18 commits merged into from Nov 16, 2022
Merged

feat: get multi records by keys #410

18 commits merged into from Nov 16, 2022

Conversation

ghost
Copy link

@ghost ghost commented Nov 6, 2022

/kind feat

What this PR does / Why we need it:
Performance optimization provided by a new endpoint to fetch records in batches by passing an array of keys.

Which issue(s) this PR fixes:
Closes #407

Special notes for your reviewer:
This new open saves interface leverage the GetMulti capabilities that was added to metaDB.

@ghost ghost requested review from hongalex and vasconcelosvcd November 6, 2022 16:20
internal/pkg/metadb/metadb.go Outdated Show resolved Hide resolved
api/open_saves.proto Outdated Show resolved Hide resolved
api/open_saves.proto Outdated Show resolved Hide resolved
@ghost ghost marked this pull request as draft November 8, 2022 00:33
@ghost ghost marked this pull request as ready for review November 8, 2022 02:36
@ghost ghost marked this pull request as draft November 8, 2022 21:43
@ghost ghost changed the title feat: get multi records by keys, streaming response feat: get multi records by keys Nov 8, 2022
@ghost
Copy link
Author

ghost commented Nov 8, 2022

After talking with Alex, we agreed that using unary gRPC message would introduce less complexity and potentially less configuration / performance issues than opening many long lived streams (which by default are capped to 100 per server).

@ghost ghost marked this pull request as ready for review November 8, 2022 22:37
@ghost ghost requested a review from hongalex November 10, 2022 16:36
internal/pkg/metadb/metadb.go Outdated Show resolved Hide resolved
api/open_saves.proto Outdated Show resolved Hide resolved
@yuryu
Copy link
Member

yuryu commented Nov 10, 2022

Added some comments here and there. To me it looks like you only need GetMultiRecords (or GetRecords) but I apologize if I'm missing the context (I'm not following the discussion), so feel free to ignore if they don't make sense. You can mark GetRecord as deprecated. Also it's probably simpler to use the multiple implementation you've added for GetRecord too.

api/open_saves.proto Outdated Show resolved Hide resolved
api/open_saves.proto Outdated Show resolved Hide resolved
internal/app/server/open_saves.go Outdated Show resolved Hide resolved
internal/app/server/open_saves_test.go Outdated Show resolved Hide resolved
internal/pkg/metadb/metadb.go Outdated Show resolved Hide resolved
internal/pkg/metadb/metadb_test.go Outdated Show resolved Hide resolved
internal/app/server/open_saves.go Outdated Show resolved Hide resolved
internal/app/server/open_saves_test.go Outdated Show resolved Hide resolved
@ghost ghost requested a review from yuryu November 15, 2022 01:11
@ghost ghost requested a review from zaratsian November 15, 2022 01:11
internal/pkg/metadb/multierror.go Outdated Show resolved Hide resolved
internal/pkg/metadb/metadb.go Outdated Show resolved Hide resolved
internal/app/server/open_saves.go Outdated Show resolved Hide resolved
@ghost ghost requested a review from hongalex November 15, 2022 14:27
Copy link
Collaborator

@vasconcelosvcd vasconcelosvcd left a comment

Choose a reason for hiding this comment

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

I have tested it against our internal server and internal tests. it looks good.

Code wise I don't see any particularity that could raise some issue.

@ghost ghost merged commit 7dbc133 into googleforgames:main Nov 16, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to fetch multiple records by keys (batch request, streaming response)
4 participants