shadow: fix crash when shadow cluster does not exist#4582
Conversation
This can happen when shadowing is used with RDS and CDS. Fixes #4504 Signed-off-by: Matt Klein <mklein@lyft.com>
| // to a CDS removal. Check that it still exists before shadowing. | ||
| // TODO(mattklein123): Optimally we would have a stat but for now just fix the crashing issue. | ||
| if (!cm_.get(cluster)) { | ||
| return; |
There was a problem hiding this comment.
I wonder if we should at least leave a warn log behind so people are not at a loss for why they're not seeing shadow traffic.
There was a problem hiding this comment.
I think a warn log is not a great idea since this is in the data path and could spam quite a bit. Though I had considered at least adding a debug log so I will do that.
There was a problem hiding this comment.
As you mention in the comment, I think a stat is most appropriate in this path. But I'm fine with merging this crash-fix as-is. @junr03 you ok with that?
| // to a CDS removal. Check that it still exists before shadowing. | ||
| // TODO(mattklein123): Optimally we would have a stat but for now just fix the crashing issue. | ||
| if (!cm_.get(cluster)) { | ||
| return; |
There was a problem hiding this comment.
As you mention in the comment, I think a stat is most appropriate in this path. But I'm fine with merging this crash-fix as-is. @junr03 you ok with that?
|
@junr03 @ggreenway I added a debug log. PTAL. |
Signed-off-by: Matt Klein <mklein@lyft.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>
3 participants
This can happen when shadowing is used with RDS and CDS.
Fixes #4504
Risk Level: Low
Testing: New UT
Docs Changes: N/A
Release Notes: N/A