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

Added some readmes and doc comments #377

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

cgillum
Copy link
Member

@cgillum cgillum commented Apr 9, 2024

These are some notes I took while watching the two recordings of the Netherite 101 sessions (both of which I unfortunately missed because I was on vacation). I thought it would be good to have them in the repo so that this additional context can be visible to anyone in an obvious location.

I suppose we could have put some of this content in the Netherite docs content, but I feel like this information is more suited for code maintainers rather than end users, hence it would make more sense to collocate it with the code.

@sebastianburckhardt I'm not 100% confident in the accuracy of the content I wrote, so it would be great if you could correct any mistakes. Feel free to suggest additional content as well. You can also just edit this PR directly if you want. :)

Copy link
Member

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

Super helpful, I feel like I learned some stuff as well - perhaps I should re-review those records as well. Left some notes, but I'll defer to Sebastian for the final call on these.

Comment on lines +13 to +15
/// <summary>
/// Update event that executes when a orchestration work item is completed.
/// </summary>
Copy link
Member

Choose a reason for hiding this comment

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

based on my assumptions, I presume a batch refers to multiple WIs.

Suggested change
/// <summary>
/// Update event that executes when a orchestration work item is completed.
/// </summary>
/// <summary>
/// Update event that executes when a orchestration work items are completed.
/// </summary>


### [Client events](./ClientEvents/)

Client events are events that are sent from the partition and *executed on the client*. In most cases, they are responses to client-initiated requests. For example, a client sends a [StateRequestReceived](./PartitionEvents/External/FromClients/StateRequestReceived.cs) event to a partition to request the state of an orchestration, and the partition responds back to the client with a [StateResponseReceived](./ClientEvents/StateResponseReceived.cs) event, containing that result.
Copy link
Member

@davidmrdavid davidmrdavid Apr 9, 2024

Choose a reason for hiding this comment

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

Something confuses me here: This says that client events are sent from the partition and executed on the client, but then you explain that StateRequestReceived is sent from the client and executed in the partition which seems contradictory.

Perhaps the way to make sense of this is that StateRequestReceived is not considered a client event? If so - I recommend we call that out for clarity's sake :-) .

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was worried that readers might be confused by this, and you confirmed it. :) Let me try to clarify this...

Comment on lines +29 to +31
### [Event fragments](./Fragments/)

(Appears to be a special type of event that allows taking large events and breaking them up into smaller events - more research is needed to confirm)
Copy link
Member

Choose a reason for hiding this comment

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

If I understand this correctly, I believe you're right. If I'm not mistaken - sometimes an event is split into multiple blobs and as such it's events are received in fragments that need to be re-assembled.

Related: https://github.com/microsoft/durabletask-netherite/blob/main/src/DurableTask.Netherite/Util/FragmentationAndReassembly.cs


## Event processing

Netherite operations that modify state are often composed of multiple events in a sequence. This is typically done because partition state needs to be loaded into the cache before it can be modified. For example, the process of creating a new orchestration or entity (instance) involves three steps:
Copy link
Member

Choose a reason for hiding this comment

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

I'm intrigued by the word "cache" here - is that particularly meaningful here? We can't we just say "loaded into memory"?

Copy link
Member Author

@cgillum cgillum Apr 9, 2024

Choose a reason for hiding this comment

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

This was the terminology that Sebastian used pretty consistently throughout the part 2 presentation, so I was trying to be consistent. That said, I too don't know if there's any distinction between "cached" and "in-memory" data and prefer the latter if there isn't a useful distinction.


* `IntakeWorker`: Assigns commit log position to each event and decides whether to send the event to a `LogWorker` or a `StoreWorker`.
* `LogWorker`: Continuously persists update events to the commit log using **FasterLog**.
* `StoreWorker`: Handles read/update/query events on the partition state, and periodically/asynchronously checkpoints to **FasterKV**.
Copy link
Member

Choose a reason for hiding this comment

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

Something worth calling out as we get into the nitty gritty - FasterKV also has an inner log (called the hybrid log) and this is important to remember in particularly hairy FASTER investigations. So when we talk about "the FASTER log" it's not always immediately clear if we mean FasterLog of FasterKV's "hybrid log". Just FYI, in case we want to sneak that info somewhere in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's interesting to know that FasterKV is internally implemented using a log, but I'll defer to Sebastian on whether we need additional clarity on the terminology here. I wouldn't expect the average Netherite developer to know the internals of FASTER, but then again, every Netherite contributor I know does know the internals of FASTER so the data already contradicts my assumptions. :)

@sebastianburckhardt sebastianburckhardt added this to the 1.5.1 milestone Apr 12, 2024
@sebastianburckhardt sebastianburckhardt removed this from the 1.5.2 milestone Jul 31, 2024
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