Add warn logs for receiving null properties #1004
Conversation
bohhyang
requested review from
nizarm,
PapaCharlie,
ZoabKapoor,
shivamgupta1,
lauraliu61b and
brycezhongqing
| @@ -77,6 +77,7 @@ protected void handlePut(final String listenTo, final ClusterProperties discover | |||
| } | |||
| else | |||
| { | |||
| _log.warn("Received a null cluster properties for {}", listenTo); | |||
There was a problem hiding this comment.
Last time we added logs around, they ended up being very noisy. Can you double check this isn't the case for this logger?
There was a problem hiding this comment.
this log should not be logged unless there is a bug. And if there is a bug, we would want to see this log and take actions. So I think this log should be a must-show. I can make it an error instead?
There was a problem hiding this comment.
I guess the Uri one could use a rate limiter in case of huge clusters having many uris. The service and cluster data won't change frequently anyway.
There was a problem hiding this comment.
A rate limit seems like a good idea. I agree we want to fail loudly, but we don't want to fail so loudly it breaks people's logs to the point where we're scrambling to put in a fix! This has happened a few times already
Summary
As the title. Add these logs for detecting potential issues and debugging. If the null guard in property event bus works as expected and xDS server sends valid data, these logs should NEVER appear (except the harmless one in FileStore).
Test Done
unit tests