*: add wire-protocol.md#38
*: add wire-protocol.md#38shubhamdhama wants to merge 1 commit intocockroachdb:stream-multiplexingfrom
Conversation
|
Checklist
|
cthumuluru-crdb
left a comment
cthumuluru-crdb
left a comment
There was a problem hiding this comment.
Reviewed only the wire-protocol.md file.
| packet being assembled), and an `assembling` flag to tell apart continuation | ||
| frames from new-message starts. | ||
|
|
||
| 1. **Stream ID must match.** A misrouted frame is a protocol error. |
There was a problem hiding this comment.
On receiving end, we wouldn't know if it is a misrouted frame. All we know is, we received a frame for a stream we don't know about. Depending on how we implement, a bug in the routing logic also can cause this. Either way, it will be a protocol error.
There was a problem hiding this comment.
Sorry I don't understand the question here. Let me elaborate: So this is a stream level constraint. That is, a stream is created and manager (reader) is dispatching a frame to that stream. So the frame must belong to that stream.
There was a problem hiding this comment.
All I'm saying is, if the packet is routed wrongly to a stream, we have an implementation bug.
wire-protocol.md
Outdated
| 2. **Message ID must not go backwards.** If the incoming frame's message ID | ||
| (`msg`) is less than `nextMessageID`, that's a monotonicity violation. | ||
| 3. **A new message (or the very first frame) starts a fresh packet.** When the | ||
| incoming frame's message ID (`msg`) is greater than `nextMessageID`, or when |
There was a problem hiding this comment.
or when the stream isn't currently assembling
If the new frame has message ID bigger than what we know so far, it should be a new frame and we must discard the data accumulated so far. Assuming we always bump the message ID once we receive done frame, I'm trying to understand how the assembling state is relevant here.
There was a problem hiding this comment.
We bump the nextMessageID when the current packet is complete, that means for next incoming frame the message ID equal to or greater than nextMessageID. So this won't satisfy incoming frame's message ID (msg) is greater than nextMessageID, but it satisfies the stream isn't currently assembling.
There was a problem hiding this comment.
A new message (or the very first frame) starts a fresh packet.** When the incoming frame's message ID (
msg) is greater thannextMessageID, or when the stream isn't currently assembling, any accumulated data is discarded and a new packet begins.
This is the most critical point I think and it may help to expand this one a bit.
A new message (or the very first frame) starts a fresh packet.
This is the first frame with a messageID >= nextMessageID right?
When the incoming frame's message ID (
msg) is greater thannextMessageID, or when the stream isn't currently assembling, any accumulated data is discarded and a new packet begins.
By 'accumulated data', do we mean, previously assembled packet data that is incomplete? If hte message ID > nextMessageID and the previous packet was complete, we don't discard any data right?
the stream isn't currently assembling
Need more clarity on when the stream is assembling vs not.
There was a problem hiding this comment.
we don't discard any data right?
Right.
Need more clarity on when the stream is assembling vs not.
Ah, yeah I should clearly call out the in the very first paragraph what does assembling means.
This is the first frame with a messageID >= nextMessageID right?
This is still very loose way to say that. Frames of same packet will have the same message ID too.
The very first frame of a new packet is messageID>nextMessageID or messageID=nextMessageID if previous frame finished the previous packet.
There was a problem hiding this comment.
The very first frame of a new packet is messageID>nextMessageID or messageID=nextMessageID if previous frame finished the previous packet.
Can we also explicitly spell out the behaviour if a frame with messageID > nextMessageID arrives after a complete packet? The spec only says that message ids need to be monotonically increasing and not necessarily continuous, so I'm thinking this would not be an error and we will simply start a new packet. If at all this is an out of order frame/packet, it will be caught whenever the previous message id arrives.
There was a problem hiding this comment.
I think my english and punctuation failed me. Let me write it this way,
A frame starts a new packet if
- Incoming frame's messageID>nextMessageID OR
- Incoming frame's messageID==nextMessageID AND we are not assembling an existing packet (that is, this is the first ever frame we are seeing for this stream or previous frame completed the last assembling packet so we are assembling a new packet).
Can you give me an example that's confusing you? Also please have a look at the examples down below.
There was a problem hiding this comment.
Not confused - just thought that this was not explicit (though I could infer it) in the description or examples.
Basically, the fact that the sequence below is acceptable.
s1 m1 d=f
s1 m1 d=t
s1 m3 d=f (Note that, we didn't receive [s1 m2] and it is possible [s1 m2] comes later and is detected as an out of order packet)
Your message above explains it perfectly. It would be great if we can summarise this thread and update the .md!
There was a problem hiding this comment.
Yeah this sequence is acceptable as per existing protocol but it doesn't have to be :) We can change it.
[s1, m2] coming later would be a protocol error in this case. This will be a bug in our DRPC codebase as TCP ensures the packets would be ordered.
There was a problem hiding this comment.
I don't see a strong reason to disallow [s1 m3] - I think we would need to allow it for the same reason that we allow it mid-packet (asynchronous interrupt).
wire-protocol.md
Outdated
| 3. **A new message (or the very first frame) starts a fresh packet.** When the | ||
| incoming frame's message ID (`msg`) is greater than `nextMessageID`, or when | ||
| the stream isn't currently assembling, any accumulated data is discarded and | ||
| a new packet begins. (Discarding mid-assembly data is inherited from the |
There was a problem hiding this comment.
Discarding mid-assembly data is inherited from the original protocol to support asynchronous interrupts. Within a stream this feels surprising and may be worth tightening later.
Maybe related to how transient errors are handled.
There was a problem hiding this comment.
I don't like this condition because we are processing a stream here. Let's say this is a client stream. Now, the client is receiving frames and building packets out of them. Now at some point, a frame [K:message, ID:100, Done:False] has arrived and after that [K:message, ID:101, Done:True] has come, it smells a rat here. What could be the possible reason previous packet wasn't complete. Packet writing doesn't have a logic to do that. The only reason a packet won't be allowed to complete is that on the server side some context has been cancelled, but that means the whole stream is closed.... OOOokayy... Now that I wrote this I figured why this might happen. The stream is stopped in between to send any more message frames (packet incomplete) but a KindClosed or KindCancel might arrive and that could still be handled by the current stream.
There was a problem hiding this comment.
Discarding mid-assembly data is inherited from the original protocol to support asynchronous interrupts.
What is the original protocol referred to here? The drpc spec? We may want to remove reference to the original protocol and just mention that this support is needed to support asynchronous interrupts.
There was a problem hiding this comment.
The stream is stopped in between to send any more message frames (packet incomplete) but a KindClosed or KindCancel might arrive and that could still be handled by the current stream.
Yeah, nice!
There was a problem hiding this comment.
What is the original protocol referred to here? The drpc spec? We may want to remove reference to the original protocol and just mention that this support is needed to support asynchronous interrupts.
Make sense. Yes it referred to the spec written on wiki and as per code.
| These live in the non-mux manager's reader loop. A future mux manager will have | ||
| its own set of rules since interleaved streams are expected there. | ||
|
|
||
| 1. **Global frame monotonicity.** Frame IDs on the wire must be non-decreasing. |
There was a problem hiding this comment.
I don't think message IDs have to be monotonous outside the context of a stream. If you take a look less() implementation on packet, message ID is expected to be increasing within the same stream. Right?
func (i ID) Less(j ID) bool {
return i.Stream < j.Stream || (i.Stream == j.Stream && i.Message < j.Message)
}
There was a problem hiding this comment.
Please see the example "### Connection rule 1: global monotonicity" → "Cross-stream:"
There was a problem hiding this comment.
Yeah, I saw that. The example looks good but calling it "global frame monotonicity" is imo not right.
There was a problem hiding this comment.
It is global frame monotonicity, isn't it? Any valid consumed frame have their IDs globally monotonically increasing? Any non-monotonic frame is either causing a protocol error or simply ignored.
There was a problem hiding this comment.
Oh I see your confusion,
message IDs have to be monotonous outside
Frame ID = Stream ID + Message ID. That's why frame can be monotonous. Not he message IDs.
wire-protocol.md
Outdated
| 3. **A new message (or the very first frame) starts a fresh packet.** When the | ||
| incoming frame's message ID (`msg`) is greater than `nextMessageID`, or when | ||
| the stream isn't currently assembling, any accumulated data is discarded and | ||
| a new packet begins. (Discarding mid-assembly data is inherited from the |
There was a problem hiding this comment.
Discarding mid-assembly data is inherited from the original protocol to support asynchronous interrupts.
What is the original protocol referred to here? The drpc spec? We may want to remove reference to the original protocol and just mention that this support is needed to support asynchronous interrupts.
wire-protocol.md
Outdated
| 2. **Message ID must not go backwards.** If the incoming frame's message ID | ||
| (`msg`) is less than `nextMessageID`, that's a monotonicity violation. | ||
| 3. **A new message (or the very first frame) starts a fresh packet.** When the | ||
| incoming frame's message ID (`msg`) is greater than `nextMessageID`, or when |
There was a problem hiding this comment.
A new message (or the very first frame) starts a fresh packet.** When the incoming frame's message ID (
msg) is greater thannextMessageID, or when the stream isn't currently assembling, any accumulated data is discarded and a new packet begins.
This is the most critical point I think and it may help to expand this one a bit.
A new message (or the very first frame) starts a fresh packet.
This is the first frame with a messageID >= nextMessageID right?
When the incoming frame's message ID (
msg) is greater thannextMessageID, or when the stream isn't currently assembling, any accumulated data is discarded and a new packet begins.
By 'accumulated data', do we mean, previously assembled packet data that is incomplete? If hte message ID > nextMessageID and the previous packet was complete, we don't discard any data right?
the stream isn't currently assembling
Need more clarity on when the stream is assembling vs not.
wire-protocol.md
Outdated
| 5. **Append frame data** to the in-progress packet buffer. | ||
| 6. **Done completes the packet.** `nextMessageID` is bumped to `msg + 1` and the | ||
| assembling flag is cleared. Any future frame with the same or lower message | ||
| ID gets rejected by rule 2. |
There was a problem hiding this comment.
Can you check if I have summarized it correctly below:
Current packet is complete - nextMessageID is bumped to msg + 1, clear assembly flag
- Next frame ID < nextMessage ID --> discarded (rule 2)
- Next frame ID == nextMessageID --> start a new packet, is the assembly flag set again?
- Next frame ID > nextMessageId --> start a new packet, is the assembly flag set again?
Current packet is not complete (no Done received yet) --> nextMessageID == msgID, assembly flag is set.
- Next frame < nextMessage ID --> discarded (rule 2)
- Next frame ID == nextMessageID --> append to current packet
- Next frame ID > nextMessageId --> discard current packet. Start a new packet.
There was a problem hiding this comment.
Next frame ID == nextMessageID --> start a new packet, is the assembly flag set again?
Yes ONLY IF if previous packet is finished.
Next frame ID > nextMessageId --> start a new packet, is the assembly flag set again?
Yes
The rest seems correct.
Document the wire protocol for assembling packets from frames. For now this only covers the packet assembly algorithm. The wire format of frames and packets is not specified and can be added later. Validation is split into stream constraints and connection constraints. Stream constraints will be common to both mux and non-mux managers, while connection constraints would differ between them.
shubhamdhama
force-pushed
the
streams-responsible-for-packaging
branch
from
888743b to
556db3f
Compare
shubhamdhama
changed the title
suj-krishnan
left a comment
suj-krishnan
left a comment
There was a problem hiding this comment.
Nice, thanks for taking this up!
The content right now is a good v1 spec for the protocol. We can iterate on it as we find more (client-specific and server-specific) constraints, as you had brought up on slack.
shubhamdhama
force-pushed
the
streams-responsible-for-packaging
branch
2 times, most recently
from
ee07ef0 to
556db3f
Compare
3 participants
Document the wire protocol for assembling packets from frames. For now this only covers the packet assembly algorithm. The wire format of frames and packets is not specified and can be added later.
Validation is split into stream constraints and connection constraints. Stream constraints will be common to both mux and non-mux managers, while connection constraints would differ between them.