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.

Sign up for GitHub

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

Jump to bottom

feat: Add TTL support to OpenSaves #458

Merged

vasconcelosvcd merged 12 commits into googleforgames:main from Zurvarian:main

Mar 13, 2025

Contributor

Zurvarian commented Mar 6, 2025

Add TTL support for both Records and Blobs via the new field ExpiresAt.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind fix
/kind feat
/kind build
/kind chore
/kind ci
/kind docs
/kind style
/kind refactor
/kind perf
/kind test

What this PR does / Why we need it:
This PR adds a new ExpiresAt field to Records and Blobs.
Also handles that the new ExpiresAt value is inherited by Blobs when these are attached to Records.

Finally, to prevent needing to add ExpiresAt to Chunks, it changes the behavior of the chunk update to delete the previously existing chunk.

Which issue(s) this PR fixes:

Closes #457

Special notes for your reviewer:

Zurvarian commented

View reviewed changes

configs/service.yaml Outdated Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/app/collector/collector.go Outdated Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/app/server/open_saves.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/app/server/open_saves.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/app/server/open_saves.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/app/server/open_saves.go

               	return stream.SendAndClose(chunk.ToProto())
               }
-              // deleteObjectOnExit deletes the object from GS if there are errors uploading the data or inserting a chunkref
-              func (s *openSavesServer) deleteObjectOnExit(ctx context.Context, path string) error {

Contributor Author

Zurvarian Mar 6, 2025

Following the pattern in this file, I've moved this helper function on top of the public function that is using it.

Zurvarian commented

View reviewed changes

internal/app/server/open_saves_test.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/app/server/open_saves_test.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/app/server/open_saves_test.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/config/loader.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/config/loader.go Outdated Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Outdated Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go

               			}
               		}
+              		// Call the custom defined updater method as well to modify the record.
+              		record, err := updater(record)

Contributor Author

Zurvarian Mar 6, 2025

I need to update the record previously to update the Blob, this is because the record could have its ExpiresAt updated, and thus we need to update the Blob's too.

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Outdated Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Outdated Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb.go Show resolved Hide resolved


          feat: Add TTL support to OpenSaves

94e1a88

Zurvarian force-pushed the main branch from e45332b to 94e1a88 Compare

March 6, 2025 14:06

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb_test.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb_test.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/metadb_test.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/app/collector/async/server.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/app/collector/async/collector.go Show resolved Hide resolved


          feat: Add Async Collector implementation based on CloudRun events

ed9b66c

Zurvarian force-pushed the main branch from ad61487 to ed9b66c Compare

March 10, 2025 09:22

Zurvarian commented

View reviewed changes

Makefile Show resolved Hide resolved

Zurvarian commented

View reviewed changes

Dockerfile Show resolved Hide resolved

Zurvarian added 3 commits

March 10, 2025 11:00


          docs: Add documentation for Async-Collector

46b4f96


          fix: Import protobuf support in Async Collector; Do not update Expire…

acd2102

…sAt if the new value is zero


          fix: Wrong check of GCS NotFound error

508f1a8

Zurvarian commented

View reviewed changes

cmd/collector/async/main.go Show resolved Hide resolved

Zurvarian marked this pull request as ready for review

March 11, 2025 15:02

Contributor Author

Zurvarian commented Mar 11, 2025

Missing UT for the new Collector.

Collaborator

lorangf commented Mar 11, 2025

/gcbrun


          fix: Fix UT in Collector, now chunks are not filtered based on their …

52f7aeb

…timestamp or state

Contributor Author

Zurvarian commented Mar 12, 2025 •

edited

Loading

@vasconcelosvcd @lorangf I've fixed the failing test though, please, wait until I add the missing UT for the Async Collector to run /gcbrun again.

EDIT: I've added the missing test.

Qohar97 reviewed

View reviewed changes

Qohar97 left a comment


          feat: Add missing UT for Async Collector; Make log level configurable…

96aa85b

… in Async Collector

Zurvarian commented

View reviewed changes

cmd/collector/async/main.go Show resolved Hide resolved

Zurvarian commented

View reviewed changes

internal/pkg/metadb/blobref/chunkref/chunkref.go Show resolved Hide resolved

Collaborator

vasconcelosvcd commented Mar 12, 2025

/gcbrun


          fix: Fix chunkref save UT

e348a4b

googleforgames deleted a comment from QoHaR-1997

Collaborator

vasconcelosvcd commented Mar 12, 2025

/gcbrun

Zurvarian requested a review from lorangf

March 12, 2025 13:52


          feat: Set chunkref.Status to be ignored on saves when its value is zero

4bd49c9

Collaborator

vasconcelosvcd commented Mar 12, 2025

/gcbrun

lorangf requested a review from vasconcelosvcd

March 13, 2025 00:47

lorangf approved these changes

View reviewed changes

Collaborator

lorangf left a comment

Looks good.

We should be create a new release and bump up the minor version at a minimum since there were some changes in the interface but nothing that is not backward compatible.

vasconcelosvcd approved these changes

View reviewed changes

vasconcelosvcd merged commit ae8df3b into googleforgames:main

4 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet