Skip to content

upstream: remove thread local cluster after callbacks#7994

Closed
ramaraochavali wants to merge 2 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/cluster_remove_crash
Closed

upstream: remove thread local cluster after callbacks#7994
ramaraochavali wants to merge 2 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/cluster_remove_crash

Conversation

@ramaraochavali
Copy link
Copy Markdown
Contributor

Description: Remove thread local cluster after clusterRemoval callback is fired. Fixes #7990.
Risk Level: Low
Testing: Updated
Docs Changes: N/A
Release Notes: N/A
Fixes #7990

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@msukalski This seems to fix the issue. Can you PTAL?

@mattklein123 Does this change make sense?

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: docker (failed build)

🐱

Caused by: a #7994 (comment) was created by @ramaraochavali.

see: more, trace.

@mattklein123
Copy link
Copy Markdown
Member

Thanks @ramaraochavali this does seem correct to me. It would make me feel better though if there was some integration test that covered this, but that might be pretty hard. @snowp any thoughts on this one?

@snowp
Copy link
Copy Markdown
Contributor

snowp commented Aug 21, 2019

Yeah makes sense to me. Maybe it would be possible to add a regression test for the Redis cluster case if setting up a dedicated test for this is too hard?

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@mattklein123 @snowp I tried to see if it is possible to add a test, it seems pretty hard to set that up. Can we merge this and I will continue to see if it is possible to add and do it as a follow-up if I can add?

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: docker (failed build)
🔨 rebuilding ci/circleci: coverage_publish (failed build)

🐱

Caused by: a #7994 (comment) was created by @ramaraochavali.

see: more, trace.

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: coverage_publish (failed build)
🔨 rebuilding ci/circleci: docker (failed build)

🐱

Caused by: a #7994 (comment) was created by @ramaraochavali.

see: more, trace.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

New commits are not reflecting here. Will close this and open another PR

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.

Envoy crashes with assertion failure on redis cluster removal

3 participants