nikhiln64 commented on PR #4316: URL: https://github.com/apache/hertzbeat/pull/4316#issuecomment-5312634147
Thanks for the thorough review Aias00, glad the fix and the tests read well. You are right on both notes. The unbounded retention is a real gap, though it is an existing property of GroupAlertCache rather than something this change introduced, since a silently stopped member and an emptied group cache were never evicted before either. I would rather not widen the scope of this bugfix, so I am happy to follow up with a max age eviction for the cache entries in a separate PR, so a member that stops being reported without a resolve event cannot keep a group firing forever. On the visibility, hertzbeat does not use a VisibleForTesting annotation anywhere in the tree today, so I left runCheckAndSendGroups package private with the explaining comment rather than pull in a new annotation, though I can switch it to whatever hook you prefer. The Backend CI and License Checker runs are sitting in action_required and need a maintainer to approve them before they start, so whenever you have a moment could you approve the run so this can go green for merge. Happy to rebase onto the latest master first if you would prefer the branch current. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
