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

Read after write #1398

Merged
merged 30 commits into from
Aug 10, 2023
Merged

Read after write #1398

merged 30 commits into from
Aug 10, 2023

Conversation

dholms
Copy link
Collaborator

@dholms dholms commented Jul 28, 2023

This is a basic implementation of read stick on the pds which gives us read-after-write semantics for some operations.

The basic approach here is:

  • add a optional logical clock to each commit in the repo
  • alongside each record, notate the logical clock at which it was added/updated
  • in appview responses, set a header (atproto-clock) which includes it's last seen logical clock for the requesting user
  • after each proxied request to the appview, check which of the user's records, if any, have not made it into the appview's indexes
  • if a relevant record has not made it in yet, add that data into the view locally

Importantly, while this includes application-specific logic, it does not include any application-specific indexes and may be done generically off of the record table.

The application of this read-sticky approach is not exhaustive. However, this applies it to some of the most common and impactful occurrences. Namely:

  • profiles in getProfile
  • profiles in getProfiles
  • posts in getPostThread
  • posts in getTimeline
  • posts and profiles in getAuthorFeed

Note: this currently builds a PDS url for any local images that get added in, however after landing #1248, we can deterministically build appview image urls

@dholms dholms marked this pull request as ready for review August 2, 2023 22:29
@bnewbold
Copy link
Collaborator

bnewbold commented Aug 2, 2023

I was a bit worried we had specified ourself in to a corner, but thankfully:

Adding optional fields to commit and MST node objects may or may not result in a repo format version change.

@ericvolp12 had the idea to use rev or revision as the field name instead of clock. I'm fine with clock after having used it a bunch, but we might be in a bubble and rev might be clearer and be back-pressure against folks assuming the TID value is a safe, trusted, general-purpose timestamp for the commit (eg, if they aren't familiar with Lamport clocks).

packages/pds/src/index.ts Outdated Show resolved Hide resolved
packages/repo/src/repo.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

🤯 a couple thoughts/questions, but you nailed it.

Comment on lines 165 to 169
for (let i = 0; i < feed.length; i++) {
if (feed[i].post.indexedAt < post.indexedAt) {
feed.splice(i, 0, { post })
inserted = true
break
Copy link
Collaborator

Choose a reason for hiding this comment

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

Spidey senses tingled a little seeing a loop over feed that also mutates feed, but this should be fine since it causes a break.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleaned it up a bit with a findIndex instead 👌

@dholms dholms merged commit 3f0e1b2 into main Aug 10, 2023
@dholms dholms deleted the read-after-write branch August 10, 2023 18:42
mloar pushed a commit to mloar/atproto that referenced this pull request Sep 26, 2023
* first pass on profiles

* quick test

* wip

* wip

* test post thread

* record embeds

* get author feed profiles

* wip timeline

* fix get timeline

* switch from counter to tid

* tidy into a service

* quick tid test

* pr feedback

* clock -> rev

* update image formatting

* disable migration & build branch

* add recent posts to getAuthorFeed & handle post thread not found errors

* refactor for lag header

* tidy

* rm collections check

* tidy test

* pr feedback

* fix small bug

* build branch

* get migrations into system

* enable migrations
mloar pushed a commit to mloar/atproto that referenced this pull request Nov 15, 2023
* first pass on profiles

* quick test

* wip

* wip

* test post thread

* record embeds

* get author feed profiles

* wip timeline

* fix get timeline

* switch from counter to tid

* tidy into a service

* quick tid test

* pr feedback

* clock -> rev

* update image formatting

* disable migration & build branch

* add recent posts to getAuthorFeed & handle post thread not found errors

* refactor for lag header

* tidy

* rm collections check

* tidy test

* pr feedback

* fix small bug

* build branch

* get migrations into system

* enable migrations
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.

3 participants