Skip to content

Conversation

alyssawilk
Copy link
Contributor

This reverts commit 1d14b9e.

Will fix the watermark issue and ask for smoke test :-)

@alyssawilk alyssawilk requested a review from mattklein123 June 11, 2018 19:12
…been received (envoyproxy#3574)"

This reverts commit 1d14b9e.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@mattklein123
Copy link
Member

See other comment, not related...

@alyssawilk alyssawilk closed this Jun 11, 2018
@alyssawilk alyssawilk reopened this Jun 11, 2018
@alyssawilk
Copy link
Contributor Author

I still think the watermark code is wrong through. I've been trying and failing to repro, but I think if the connection is aboveHighWatermark() on stream creation, we should track that immediately and not delay the check until filter chain creation, because if the state changes, I believe the stream will still get the lowWatermark callback without having gotten the high watermark callback and it'll screw up accounting.

@mattklein123
Copy link
Member

OK SGTM, let's revert if we think there is a bug here.

@alyssawilk alyssawilk merged commit 28cea91 into envoyproxy:master Jun 11, 2018
alyssawilk added a commit to alyssawilk/envoy that referenced this pull request Jun 12, 2018
…rs have been received (envoyproxy#3574)" (envoyproxy#3590)"

This reverts commit 28cea91.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk deleted the revert branch October 10, 2018 19:56
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.

2 participants