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

fix!: remove indexing for timestamps #399

Merged
merged 10 commits into from
Oct 12, 2022

Conversation

hongalex
Copy link
Collaborator

@hongalex hongalex commented Oct 12, 2022

What type of PR is this?

/kind fix

What this PR does / Why we need it:
Removes indexing for all timestamps since it breaks best practices around monotonically increasing values.

Which issue(s) this PR fixes:

Special notes for your reviewer:
This requires changes on the Open Saves collector, which previously depended on the use of the Timestamps fields to select entities older than a certain time. Now, we fetch all blobs/chunks with a status and only selectively delete.

This is also a breaking change to QueryRecords since we will no longer support querying by ascending/descending timestamps.

@hongalex hongalex requested review from a user, lorangf and yuryu October 12, 2022 06:28
@hongalex hongalex changed the title fix: remove indexing for timestamps fix!: remove indexing for timestamps Oct 12, 2022
@hongalex
Copy link
Collaborator Author

The tests that are failing are only part of the collector and they are failing on main as well.

@hongalex
Copy link
Collaborator Author

Fixed the failing collector tests. PTAL @loranger2k

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good.

We may want to eventually update the index.yaml examples to use the record key as a prefix to follow best practices and discourage the use of monotonically increasing properties are strongly discouraged.

@hongalex hongalex merged commit 9b47ab6 into googleforgames:main Oct 12, 2022
@hongalex hongalex deleted the remove-timestamp-index branch October 12, 2022 17:38
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.

1 participant