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

Event log #290

Merged
merged 24 commits into from
Apr 8, 2023
Merged

Event log #290

merged 24 commits into from
Apr 8, 2023

Conversation

mistermoe
Copy link
Member

@mistermoe mistermoe commented Mar 31, 2023

First of 3 PRs to implement sync. Introduces an EventLog interface along with a concrete level-based implementation. Events are added to the EventLog for:

  • RecordsWrite
  • RecordsDelete
  • ProtocolsConfigure

The PR that follows this one will introduce the EventsGet interface method

src/event-log/event-log-level.ts Outdated Show resolved Hide resolved
src/event-log/event-log-level.ts Outdated Show resolved Hide resolved
src/event-log/event-log-level.ts Outdated Show resolved Hide resolved
src/event-log/event-log-level.ts Outdated Show resolved Hide resolved
src/event-log/event-log-level.ts Outdated Show resolved Hide resolved
src/event-log/event-log-level.ts Outdated Show resolved Hide resolved
src/event-log/event-log.ts Outdated Show resolved Hide resolved
src/store/storage-controller.ts Outdated Show resolved Hide resolved
tests/event-log/event-log-level.spec.ts Outdated Show resolved Hide resolved
src/event-log/event-log.ts Show resolved Hide resolved
@mistermoe mistermoe linked an issue Apr 1, 2023 that may be closed by this pull request
Copy link

@diehuxx diehuxx left a comment

Choose a reason for hiding this comment

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

Codewise this looks generally good to me, with a couple small questions. But I have some broader design questions before I stamp.

Let me know if you'd prefer design discussion here or in your tech proposal.

Trying to think through this scenario.

  1. Say that DWN A and B both have record R. Initially they have the same most recent update of record R.
  2. Then DWN A receives a RecordsWrite(A) updating R to be R(A). Slightly after that, before any sync occurs, DWN B also receives a RecordsWrite(B) updating R to be R(B).
  3. Is it deterministic which version of R wins? If not, is that acceptable?

If DWN A initiates a pull from B, then it will send the CID of RecordsWrite(A) to DWN B in an EventsGet. DWN B has never seen this message, so I’m actually not sure what it will return, since it’s supposed to return events after the watermark. Does it return no events? Or all events from all time?

src/dwn.ts Show resolved Hide resolved
src/event-log/event-log-level.ts Show resolved Hide resolved
src/event-log/event-log-level.ts Outdated Show resolved Hide resolved
src/event-log/event-log-level.ts Outdated Show resolved Hide resolved
src/event-log/event-log-level.ts Outdated Show resolved Hide resolved
src/event-log/event-log-level.ts Show resolved Hide resolved
src/event-log/event-log-level.ts Show resolved Hide resolved
src/store/storage-controller.ts Outdated Show resolved Hide resolved
src/dwn.ts Outdated Show resolved Hide resolved
@mistermoe
Copy link
Member Author

@thehenrytsai i went ahead and included functional tests. pls lmk if you'd rather them go elsewhere.

@mistermoe
Copy link
Member Author

just realized that we'll need to delete events when a protocol is overwritten as well. Specifically, here. 😭

@mistermoe
Copy link
Member Author

just realized that we'll need to delete events when a protocol is overwritten as well. Specifically, here. 😭

done

@mistermoe
Copy link
Member Author

does anyone have any reservations left here?

@thehenrytsai
Copy link
Member

The only reservation for me is that the code does a full scan of the event log for every clobber, but fine if we create an issue to improve event deletion by CIDs as a separate item to unblock the merge of this PR.

@mistermoe
Copy link
Member Author

@thehenrytsai i'm now storing events in watermark order and cid order. this prevents us from having to scan a tenant's entire event log when deleting by CID. cost incurred is 2x storage for events. the size of each event is numDidBytes + numCidBytes.

requests to change anything else about EventLogLevel can be submitted as separate issues that can be tackled later. I'd like to get this PR merged so that i can move to the next sync-related PR.

@csuwildcat
Copy link
Contributor

Sounds like a good plan to me, Moe

Copy link
Member

@thehenrytsai thehenrytsai left a comment

Choose a reason for hiding this comment

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

🥇 🙏 💯 🎸

@mistermoe mistermoe merged commit f343ab3 into main Apr 8, 2023
@mistermoe mistermoe deleted the event-log branch April 8, 2023 05:05
@mistermoe mistermoe mentioned this pull request Apr 8, 2023
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.

Implement Sync
5 participants