| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 84
Add ttrpc protocol definition #102
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/easy/light/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The protocol is designed to make low-overhead implementations as simple as possible"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe always enclose local close and remote closed in backticks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to be more consistent on referring to those states
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <derek@mcg.dev>
dmcgowan
force-pushed
the
protocol-definition
branch
from
00f9546 to
30684b0
Compare
|
Updated, this should be ready to go now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's merge!
Adds a more formalized protocol definition.
Expands the definition for streaming support. The implementation for streaming support is still a work in progress.