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.

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

Watch improvements in datastore #1681

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Watch improvements in datastore #1681

merged 4 commits into from
Dec 18, 2023

Conversation

josephschorr
Copy link
Member

Changes the Watch implementations to support a combined relationships, schema and checkpoint set of events

@josephschorr josephschorr requested a review from ecordell December 8, 2023 21:05
@josephschorr
Copy link
Member Author

I'll be adding additional integration tests on Monday, but putting this out for review in the meantime

@github-actions github-actions bot added area/api v1 Affects the v1 API area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Dec 8, 2023
@josephschorr josephschorr force-pushed the watch-improvements branch 2 times, most recently from 0e6ee51 to 44639fb Compare December 11, 2023 19:59
@josephschorr josephschorr marked this pull request as ready for review December 11, 2023 19:59
@josephschorr josephschorr requested a review from a team December 11, 2023 19:59
@github-actions github-actions bot added the area/schema Affects the Schema Language label Dec 12, 2023
vroldanbet
vroldanbet reviewed Dec 12, 2023
View reviewed changes
Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

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

First quick pass

internal/datastore/context.go Show resolved Hide resolved
internal/datastore/memdb/memdb_test.go Show resolved Hide resolved
internal/datastore/memdb/watch.go Outdated Show resolved Hide resolved
internal/datastore/common/changes.go Show resolved Hide resolved
internal/datastore/common/changes.go Show resolved Hide resolved
internal/datastore/common/changes.go Outdated Show resolved Hide resolved
internal/datastore/common/changes.go Outdated Show resolved Hide resolved
internal/datastore/common/changes.go Show resolved Hide resolved
@github-actions github-actions bot removed the area/schema Affects the Schema Language label Dec 12, 2023
@josephschorr
Copy link
Member Author

Rebased

@github-actions github-actions bot added the area/CLI Affects the command line label Dec 13, 2023
@josephschorr josephschorr force-pushed the watch-improvements branch 2 times, most recently from 256a0da to 5f0273f Compare December 13, 2023 19:31
vroldanbet
vroldanbet reviewed Dec 14, 2023
View reviewed changes
Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

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

moar comments

internal/datastore/common/changes.go Outdated Show resolved Hide resolved
internal/datastore/crdb/watch.go Show resolved Hide resolved
internal/datastore/crdb/watch.go Show resolved Hide resolved
internal/datastore/crdb/watch.go Show resolved Hide resolved
internal/datastore/crdb/watch.go Show resolved Hide resolved
internal/datastore/common/changes.go Show resolved Hide resolved
internal/datastore/common/changes.go Outdated Show resolved Hide resolved
internal/datastore/crdb/watch.go
}
}

if opts.Content&datastore.WatchCheckpoints == datastore.WatchCheckpoints {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, why sending a checkpoint if a non-empty changeset existed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it was requested; we always send the checkpoint

Copy link
Contributor

@vroldanbet vroldanbet Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

why do we need to send always the checkpoint? isn't it unnecessary work / CPU time in the client ?

internal/datastore/crdb/watch.go Show resolved Hide resolved
internal/datastore/crdb/watch.go Show resolved Hide resolved
@josephschorr
Copy link
Member Author

Updated

vroldanbet
vroldanbet reviewed Dec 14, 2023
View reviewed changes
internal/datastore/postgres/watch.go Outdated Show resolved Hide resolved
pkg/datastore/test/watch.go Show resolved Hide resolved
internal/datastore/common/changes_test.go Outdated Show resolved Hide resolved
internal/datastore/crdb/watch.go
}
}

if opts.Content&datastore.WatchCheckpoints == datastore.WatchCheckpoints {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we don't have a test for checkpoints - run all CRDB tests with code coverage

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't because they aren't used externally yet

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still code written and it's at risk of being forgotten down the line.

internal/datastore/common/changes.go Show resolved Hide resolved
internal/datastore/common/changes.go Show resolved Hide resolved
internal/datastore/common/changes.go Show resolved Hide resolved
internal/datastore/postgres/watch.go Outdated Show resolved Hide resolved
internal/datastore/postgres/watch.go Outdated Show resolved Hide resolved
internal/datastore/spanner/migrations/zz_migration.0007_register_combined_change_stream.go Show resolved Hide resolved
vroldanbet
vroldanbet reviewed Dec 15, 2023
View reviewed changes
Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

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

Finally finished reviewing everything, thanks for your patience 🙏🏻

internal/datastore/proxy/schemacaching/watchingcache.go Outdated Show resolved Hide resolved
internal/datastore/proxy/schemacaching/watchingcache.go Outdated Show resolved Hide resolved
internal/datastore/postgres/watch.go Show resolved Hide resolved
internal/datastore/proxy/schemacaching/watchingcache.go Show resolved Hide resolved
internal/datastore/spanner/watch.go Outdated Show resolved Hide resolved
internal/datastore/spanner/watch.go Show resolved Hide resolved
internal/datastore/spanner/watch.go Show resolved Hide resolved
@josephschorr
Remove WatchSchema and adjust existing Watch API in prep for a combin…
5e87b66 
…ed Watch API supporting rels, schema and checkpoints
@josephschorr
Add support for schema updates and checkpoint updates to:
e000805 
- CRDB
- Postgres
- Spanner

The other datastores will simply report an error if asked for schema updates in Watch
@josephschorr
Allow for configuration of the checkpoint heartbeat in watch and wire…
f34c9b0 
… up schema watch to use the command line flag for all supported datastores
@josephschorr
Copy link
Member Author

Updated

vroldanbet
vroldanbet previously approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@vroldanbet vroldanbet left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing all the feedback, just one question

internal/datastore/spanner/options.go Outdated Show resolved Hide resolved
@josephschorr josephschorr added this pull request to the merge queue Dec 18, 2023
Merged via the queue into authzed:main with commit 1bd9932 Dec 18, 2023
20 checks passed
@josephschorr josephschorr deleted the watch-improvements branch December 18, 2023 18:11
@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vroldanbet vroldanbet vroldanbet approved these changes

@ecordell ecordell Awaiting requested review from ecordell

Assignees
No one assigned
Labels
area/api v1 Affects the v1 API area/CLI Affects the command line area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@josephschorr @ecordell @vroldanbet