Skip to content

Fix connection reuse by delaying a poll cycle.#73

Merged
jplevyak merged 5 commits intoistio:release-1.1from
jplevyak:istio-connection-reuse-fix
May 31, 2019
Merged

Fix connection reuse by delaying a poll cycle.#73
jplevyak merged 5 commits intoistio:release-1.1from
jplevyak:istio-connection-reuse-fix

Conversation

@jplevyak
Copy link

Signed-off-by: John Plevyak jplevyak@gmail.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Delay connection reuse until we have done a poll cycle to detect closed connections.
Risk Level: low
Testing: hand
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue] envoyproxy#13848
[Optional Deprecated:]

jplevyak added 2 commits May 29, 2019 14:17
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Copy link

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

/lgtm except inline comment

@lambdai
Copy link

lambdai commented May 30, 2019

@jplevyak The code looks good. I will build a proxy image. Could you add test cases before the merge?

jplevyak added 2 commits May 30, 2019 09:23
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
@jplevyak jplevyak requested review from PiotrSikora and duderino May 30, 2019 20:10
Copy link

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

The delay client is not transforming itself to ready if there is no further delayed client.
Could you add another timer? See inline comment

And the test?

if (client.delayed_ == 0) {
ENVOY_CONN_LOG(debug, "moving from delay to ready", *client.codec_client_);
client.moveBetweenLists(delayed_clients_, ready_clients_);
}
Copy link

Choose a reason for hiding this comment

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

could you create another time here when

  1. the delayed_clients is not empty, and
  2. there is no pending timer
    So that the delayed clients can be moved to ready in next batch.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: John Plevyak <jplevyak@gmail.com>
@duderino
Copy link

/lgtm
/approve

@Stono
Copy link

Stono commented May 31, 2019

/lgtm
Tested in a production environment with 5 applications for a 24 hour period.

@jplevyak jplevyak merged commit bc63ae7 into istio:release-1.1 May 31, 2019
rlenglet pushed a commit that referenced this pull request Aug 13, 2019
* Add header size, response code and destination port

Signed-off-by: Pengyuan Bian <bianpengyuan@google.com>
brian-avery pushed a commit that referenced this pull request Jun 30, 2020
Signed-off-by: Yan Avlasov <yavlasov@google.com>
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.

4 participants