Skip to content

core: dispatcher test suite and stream completion#304

Merged
goaway merged 17 commits intomasterfrom
junr03/stream-completion
Aug 15, 2019
Merged

core: dispatcher test suite and stream completion#304
goaway merged 17 commits intomasterfrom
junr03/stream-completion

Conversation

@junr03
Copy link
Member

@junr03 junr03 commented Jul 25, 2019

Description: This PR adds an initial test suite for the Dispatcher implementation. This test suite is a step in allowing us to iterate on the remaining issues (stream completion, data races, buffer passing, etc) with more confidence. Additionally this PR add logic to track stream state (local and remote) in order to track stream completion state. Once the stream is completed (both local and remote are closed) the stream is removed from the http dispatcher's stream map.

Risk Level: medium - this PR takes into account stream state to modify the http dispatchers stream map.
Testing: added initial suite of unit tests.
Fixes #302

Signed-off-by: Jose Nino jnino@lyft.com

Jose Nino added 4 commits July 25, 2019 15:37
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Jose Nino added 6 commits July 29, 2019 13:51
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
This reverts commit 9a32ed2.

Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03 junr03 changed the title core: handle stream completion core: dispatcher test suite and stream completion Jul 30, 2019
@junr03
Copy link
Member Author

junr03 commented Jul 30, 2019

Alright, this is ready for review @goaway. Note per our discussions that the implementation detail of how this PR tracks stream completion state (via local booleans right now) is subject to change once we merge envoyproxy/envoy#7752. However, the test suite is completely agnostic of this fact.

@junr03 junr03 marked this pull request as ready for review July 30, 2019 20:31
Jose Nino added 3 commits July 30, 2019 16:47
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Member Author

junr03 commented Aug 2, 2019

This PR depends on and is currently failing because this Envoy PR has not merged. As soon as that PR merges this PR should point to Envoy upstream master and get merged.

@stale
Copy link

stale bot commented Aug 9, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label Aug 9, 2019
Signed-off-by: Jose Nino <jnino@lyft.com>
@stale stale bot removed the stale label Aug 15, 2019
Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Member Author

junr03 commented Aug 15, 2019

@goaway this is up-to-date. Do you mind reviewing?

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
Copy link
Contributor

@goaway goaway left a comment

Choose a reason for hiding this comment

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

looks great!

@goaway goaway merged commit fcbc0e3 into master Aug 15, 2019
@junr03 junr03 deleted the junr03/stream-completion branch August 20, 2019 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

core: handle stream completion

2 participants