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

quic: remove duplicate code in QUIC encodeData(), encodeMetadata() and encodeTrailers() implementations #32570

Merged
merged 11 commits into from
Feb 28, 2024
Merged

quic: remove duplicate code in QUIC encodeData(), encodeMetadata() and encodeTrailers() implementations #32570

merged 11 commits into from
Feb 28, 2024

Conversation

RyanTheOptimist
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist commented Feb 26, 2024 •
edited
Loading

quic: remove duplicate code in QUIC encodeData(), encodeMetadata() and encodeTrailers() implementations

Add a QuicSpdyStream& member to EnvoyQuicStream so that the encodeData(), encodeMetadata() and encodeTrailers() implementation can be moved to EnvoyQuicStream and out of the subclasses.

Risk Level: Low
Testing: Existing
Docs Changes: N/A
Release Notes: N/A

@RyanTheOptimist
quic: remove duplciate code in QUIC encodeData() implementations
906b525 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
Cleanup
99b61c0 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
More cleanup
81ea8fc 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
Format
775b6fd 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
Add
c9ce16e 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
Datagrams
2afb849 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
Fomat
67906d5 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
Formatter
54559d0 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
Format
048ba50 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
Copy link
Contributor Author

/assign @danzh2010

@RyanTheOptimist
encodeMetadata
0b8f0da 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist
encodeTrailers
702170e 
Signed-off-by: Ryan Hamilton <[email protected]>
@RyanTheOptimist RyanTheOptimist changed the title quic: remove duplciate code in QUIC encodeData() implementations quic: remove duplciate code in QUIC encodeData(), encodeMetadata() and encodeTrailers() implementations Feb 27, 2024
@RyanTheOptimist RyanTheOptimist changed the title quic: remove duplciate code in QUIC encodeData(), encodeMetadata() and encodeTrailers() implementations quic: remove duplicate code in QUIC encodeData(), encodeMetadata() and encodeTrailers() implementations Feb 27, 2024
@RyanTheOptimist
Copy link
Contributor Author

@danzh2010 friendly ping

@mattklein123 mattklein123 self-assigned this Feb 27, 2024
@mattklein123
Copy link
Member

/wait-any

danzh2010
danzh2010 reviewed Feb 27, 2024
View reviewed changes
Copy link
Contributor

@danzh2010 danzh2010 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is a good movement! Thanks for doing it!

Just one question, is it possible to also move part of encodeHeaders(), i.e. ScopedWatermarkBufferUpdater, byte tracker and local_end_stream_ to EnvoyQuicStream?

@danzh2010
Copy link
Contributor

This is a good movement! Thanks for doing it!

Just one question, is it possible to also move part of encodeHeaders(), i.e. ScopedWatermarkBufferUpdater, byte tracker and local_end_stream_ to EnvoyQuicStream?

Discussed offline, this can be done as a follow up.

@RyanTheOptimist
Copy link
Contributor Author

This is a good movement! Thanks for doing it!
Just one question, is it possible to also move part of encodeHeaders(), i.e. ScopedWatermarkBufferUpdater, byte tracker and local_end_stream_ to EnvoyQuicStream?

Discussed offline, this can be done as a follow up.

Great, thanks! Over to you, @mattklein123

@RyanTheOptimist
Copy link
Contributor Author

/retest

@mattklein123 mattklein123 merged commit c7ce585 into envoyproxy:main Feb 28, 2024
54 checks passed
steveWang added a commit to steveWang/envoy that referenced this pull request Mar 6, 2024
@steveWang
Delete unused runtime flag.
9ea4fa3 
envoy.reloadable_features.quiche_use_mem_slice_releasor_api is no
longer used as of envoyproxy#32570.

Signed-off-by: Steve Wang <[email protected]>
@steveWang steveWang mentioned this pull request Mar 6, 2024
Merged
RyanTheOptimist pushed a commit that referenced this pull request Mar 6, 2024
@steveWang
Delete unused runtime flag. (#32739)
90846c6 
envoy.reloadable_features.quiche_use_mem_slice_releasor_api is no
longer used as of #32570.

Signed-off-by: Steve Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@danzh2010 danzh2010 danzh2010 approved these changes

Assignees

@mattklein123 mattklein123

@danzh2010 danzh2010

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RyanTheOptimist @mattklein123 @danzh2010 @KBaichoo