placate shutdown logging #260
Conversation
There was a problem hiding this comment.
Hi @dorjesinpo, what do you think about adding [THROTTLED] in every throttled log?
Good idea. Let me put that in a macro... |
|
|
||
| #define LOGTHROTTLE_INFO() \ | ||
| BALL_LOGTHROTTLE_INFO(k_MAX_INSTANT_MESSAGES, k_NS_PER_MESSAGE) \ | ||
| << "[THROTTLED] " |
There was a problem hiding this comment.
I like the idea of this macro being just near these constants
There are 3 things to consider here:
- I believe in similar cases we made
#undefeven in.cppfiles, when we define some custom macro, at least for local defines - Might be good to place this macro definition a few lines below, so it will be under the
k_NS_PER_MESSAGEconstant which it uses - A small concern about
LOGTHROTTLE_INFObeing a substring ofBALL_LOGTHROTTLE_INFO. In general, this macro name does not mention the component where it was defined, so it's a general purpose name which is not very stable when using macros
There was a problem hiding this comment.
Makes sense. Amending
| @@ -74,6 +74,10 @@ const bsls::Types::Int64 k_NS_PER_MESSAGE = | |||
| bdlt::TimeUnitRatio::k_NANOSECONDS_PER_MINUTE / k_MAX_INSTANT_MESSAGES; | |||
| // Time interval between messages logged with throttling. | |||
|
|
|||
| #define LOGTHROTTLE_INFO() \ | |||
| BALL_LOGTHROTTLE_INFO(k_MAX_INSTANT_MESSAGES, k_NS_PER_MESSAGE) \ | |||
| << "[THROTTLED] " | |||
There was a problem hiding this comment.
Overall, what do you thing about defining this macro in one of our helper headers, with an unique prefix?
Do we actually need to make these constants k_MAX_INSTANT_MESSAGES/k_NS_PER_MESSAGE different?
There was a problem hiding this comment.
Overall, what do you thing about defining this macro in one of our helper headers, with an unique prefix?
Do we actually need to make these constants
k_MAX_INSTANT_MESSAGES/k_NS_PER_MESSAGEdifferent?
No opinion on this. For now, let's not centralize this
dorjesinpo
force-pushed
the
fix/placate-shutdown
branch
from
4f8414a to
add724d
Compare
Signed-off-by: dorjesinpo <[email protected]>
Signed-off-by: dorjesinpo <[email protected]>
dorjesinpo
force-pushed
the
fix/placate-shutdown
branch
from
add724d to
854aeca
Compare
* placate shutdown logging Signed-off-by: dorjesinpo <[email protected]> * Adding [THROTTLED] prefix Signed-off-by: dorjesinpo <[email protected]> --------- Signed-off-by: dorjesinpo <[email protected]>
* placate shutdown logging Signed-off-by: dorjesinpo <[email protected]> * Adding [THROTTLED] prefix Signed-off-by: dorjesinpo <[email protected]> --------- Signed-off-by: dorjesinpo <[email protected]>
* placate shutdown logging Signed-off-by: dorjesinpo <[email protected]> * Adding [THROTTLED] prefix Signed-off-by: dorjesinpo <[email protected]> --------- Signed-off-by: dorjesinpo <[email protected]>
When having 1000's client sessions, shutting down broker can exceed shutdown timeouts and stop the Dispatcher with unfinished work. That causes an assert.
This PR does not address this completely. It increases timeouts and throttles logging. We need to think about better fix, for example:
or