Conversation
|
We can also put this in a separate markdown document or even in the Readme. Initially I was thinking for showing up in go doc, but there is more than just the go implementation now. |
|
btw, if we put this in a markdown: https://github.blog/2022-02-14-include-diagrams-markdown-files-mermaid/ You can add diagrams inline in the markdown. |
kzys
left a comment
kzys
left a comment
There was a problem hiding this comment.
Looks good to me. It may be better to have a plain Markdown file in a separate directory for utilizing Markdown-related tooling though.
dmcgowan
force-pushed
the
protocol-definition
branch
from
c6159de to
a64f783
Compare
|
@cpuguy83 I saw news about that and am definitely interested in that. Initially was just trying for a nice txt friendly format, but I can play around with it. Moved to a markdown file, I agree it is the better approach. I also need to update the versioning some since I see we already have a 1.1 |
dmcgowan
force-pushed
the
protocol-definition
branch
from
a64f783 to
eed841a
Compare
|
Updated version, going to mark as ready for review |
PROTOCOL.md
Outdated
| The ttrpc protocol is designed to be lightweight and optimized for low latency | ||
| and reliable connections between processes on the same host. The protocol does | ||
| not include features for handling unreliable connections such as handshakes, | ||
| resets, pings, or flow control. The protocol is designed to be easy to implement |
There was a problem hiding this comment.
The statement is intending to say that low overhead implementations are easy. Not sure about the wording and I can see that isn't exactly clear. Replacing easy with light doesn't convey that point though.
There was a problem hiding this comment.
"The protocol is designed to make low-overhead implementations as simple as possible"
PROTOCOL.md
Outdated
| streams since both client and server need to track additional state. To keep | ||
| this management as simple as possible, ttrpc minimizes the number of states and | ||
| uses two flags instead of control frames. Each stream has two states while a | ||
| stream is still alive, local closed and remote closed. Each peer considers |
There was a problem hiding this comment.
"alive: local closed and remote closed."
| both local and remote closed, the stream is considered finished and may be | ||
| cleaned up. | ||
|
|
||
| Due to the asymmetric nature of the current protocol, a client should |
There was a problem hiding this comment.
Maybe always enclose local close and remote closed in backticks?
There was a problem hiding this comment.
Updated to be more consistent on referring to those states
stevvooe
left a comment
stevvooe
left a comment
There was a problem hiding this comment.
LGTM
Took a quick pass and it looks reasonable.
dmcgowan
force-pushed
the
protocol-definition
branch
2 times, most recently
from
986dee9 to
c227b9c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
- Coverage 63.87% 63.67% -0.20%
==========================================
Files 11 11
Lines 1049 1049
==========================================
- Hits 670 668 -2
- Misses 334 336 +2
Partials 45 45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dmcgowan
force-pushed
the
protocol-definition
branch
from
c227b9c to
00f9546
Compare
Signed-off-by: Derek McGowan <[email protected]>
dmcgowan
force-pushed
the
protocol-definition
branch
from
00f9546 to
30684b0
Compare
|
Updated, this should be ready to go now |
7 participants
Adds a more formalized protocol definition.
Expands the definition for streaming support. The implementation for streaming support is still a work in progress.