Skip to content

Conversation

@bneradt
Copy link
Contributor

@bneradt bneradt commented Oct 20, 2022

This continues the work of #9084 with more renaming of HTTP/2 connection and stream variables and functions like so:

  • client -> peer
  • server -> local

@bneradt bneradt added the HTTP/2 label Oct 20, 2022
@bneradt bneradt added this to the 10-Dev milestone Oct 20, 2022
@bneradt bneradt requested a review from maskit October 20, 2022 01:17
@bneradt bneradt self-assigned this Oct 20, 2022
Copy link
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

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

I feel like local / remote is a more natural combination, but I'm not a best person to choose English words. I believe your choice.

I approve this PR for now, but we have remote_hpack_handle in Http2ConnectionState. Please rename it as well if you think renaming it makes sense.

@bneradt
Copy link
Contributor Author

bneradt commented Oct 20, 2022

Thanks for the review.

I feel like local / remote is a more natural combination, but I'm not a best person to choose English words. I believe your choice.

That's true. Often local and remote do go together. But I think in this case peer is better than remote for what these variables reference. The HTTP/2 RFC generally uses the term "peer" when referring to the other host:
https://www.rfc-editor.org/rfc/rfc9113.html

The term "remote" is used in the RFC, but generally to qualify peer (i.e., it says "remote peer" sometimes), or in reference in a few places to the name of a state. But in general, when discussing the other host, it calls it a "peer".

Also, #9084 already started the trend of renaming the client to peer rather than remote. So if we did choose remote instead of peer, we would have to change those places too.

I approve this PR for now, but we have remote_hpack_handle in Http2ConnectionState. Please rename it as well if you think renaming it makes sense.

Good catch. Let's rename that too.

This continues the work of apache#9084 with more renaming of HTTP/2 connection
and stream variables and functions like so:

* client -> peer
* server -> local
@bneradt bneradt force-pushed the more_h2_client_server_renames branch from ab49b73 to ef63db2 Compare October 20, 2022 03:44
@maskit
Copy link
Member

maskit commented Oct 20, 2022

Okay, let's stick with the terms that are used in RFC.
The new commit didn't stale my review somehow. Please go ahead and merge this PR.

@bneradt bneradt merged commit 5cefa56 into apache:10-Dev Oct 20, 2022
@bneradt bneradt deleted the more_h2_client_server_renames branch October 20, 2022 14:31
@zwoop zwoop modified the milestones: 10-Dev, 10.0.0 Feb 2, 2023
JosiahWI pushed a commit to JosiahWI/trafficserver that referenced this pull request Jul 19, 2023
This continues the work of apache#9084 with more renaming of HTTP/2 connection
and stream variables and functions like so:

* client -> peer
* server -> local

(cherry picked from commit 5cefa56)

Conflicts:
      proxy/http2/Http2ConnectionState.cc
      proxy/http2/Http2Stream.cc
      proxy/http2/Http2Stream.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants