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.

Sign up for GitHub

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

Jump to bottom

Tiny update to protocol overview doc. #26396

Merged
merged 5 commits into from
Sep 7, 2021
Merged

Tiny update to protocol overview doc. #26396

merged 5 commits into from
Sep 7, 2021

Conversation

morgwai
Copy link
Contributor

@morgwai morgwai commented Jun 1, 2021

Clarify client stream closing in the abstract protcol and link to more detalied HTTP/2 specific doc.

@donnadionne

markdroth
markdroth requested changes Jun 1, 2021
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

@ejona86, we really ought to finish up #15460 and get it merged, since I think that would be a much clearer way to document some of this.

CONCEPTS.md Outdated Show resolved Hide resolved
CONCEPTS.md Outdated Show resolved Hide resolved
CONCEPTS.md Outdated Show resolved Hide resolved
@markdroth markdroth self-assigned this Jun 1, 2021
@markdroth markdroth added the release notes: no Indicates if PR should not be in release notes label Jun 1, 2021
@morgwai
Copy link
Contributor Author

morgwai commented Jun 2, 2021 •
edited
Loading

thanks for the comments! I resolved 2 of them the way you suggested (although not sure if I agree, but these are just details not worth further discussion).
Please have a look at my reply regarding "by means of an underlying lower level protocol": this part is what motivated me to submit this PR as previously I spent almost 2 hours investigating how it works (see https://groups.google.com/g/grpc-io/c/M2sCDFw8vT8/m/pM_vd7kJBgAJ ), so I wanted to spare this effort for anyone else wondering about it in the future.

@morgwai morgwai requested a review from markdroth June 2, 2021 16:06
@morgwai
Copy link
Contributor Author

morgwai commented Jun 2, 2021

I've added a brief explanation of half-closing in the HTTP/2 section as well: hope now it's clearer.

@stale
Copy link

stale bot commented Sep 3, 2021

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 30 days. It will be closed automatically if no further update occurs in 7 day. Thank you for your contributions!

markdroth
markdroth reviewed Sep 3, 2021
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay in responding! This somehow fell off of my radar.

Overall, this looks good. Just a couple of remaining grammatical changes before we can get this merged.

CONCEPTS.md Outdated Show resolved Hide resolved
CONCEPTS.md Outdated Show resolved Hide resolved
@stale stale bot removed the disposition/stale label Sep 3, 2021
@morgwai
Copy link
Contributor Author

morgwai commented Sep 3, 2021

Sorry for the long delay in responding! This somehow fell off of my radar.

No worries: it's nothing urgent after all ;-)

@morgwai morgwai requested a review from markdroth September 3, 2021 16:06
@morgwai
Copy link
Contributor Author

morgwai commented Sep 6, 2021

@markdroth friendly ping in case it got lost again ;-)

@markdroth
Copy link
Member

The "Distribution Tests Python Linux" failure is an infrastructure timeout.

Known issues: #26611

@markdroth markdroth merged commit 483e11d into grpc:master Sep 7, 2021
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Sep 7, 2021
lidizheng pushed a commit to lidizheng/grpc that referenced this pull request Sep 23, 2021
@morgwai @lidizheng
Tiny update to protocol overview doc. (grpc#26396)
2f48db1 
* Tiny update to protocol overview doc.

Clarify client stream closing and link to more detalied HTTP/2 specific doc.

* apply review comments

* add brief HTTP/2 client stream closing mechanism explenation

* reword to differentiate HTTP/2 stream from gRPC message stream

* apply review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markdroth markdroth markdroth approved these changes

Assignees

@markdroth markdroth

Labels
imported Specifies if the PR has been imported to the internal repository release notes: no Indicates if PR should not be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@morgwai @markdroth @grpc-kokoro