feat(#5352): healthgroups endpoint#5416
Conversation
| * specific data in {@link HealthGroupsCache} on receiving an | ||
| * {@link InstanceDeregisteredEvent}. | ||
| */ | ||
| public class HealthGroupsCacheCleanupTrigger extends AbstractEventHandler<InstanceDeregisteredEvent> { |
There was a problem hiding this comment.
This seems a bit an overkill to me.
It would be enough to invoke the deletion in the StatusUpdater with a doOnNext as it's already done to persist the groups (actually, it's enough to add an if there to check the event type and either call the save or the delete method).
We're scheduling a thread to constantly check for a single even type, which barely happens, where the StatusUpdater is already handling all of them anyway.
There was a problem hiding this comment.
StatusUpdater polls the /health endpoint — it does not receive InstanceDeregisteredEvent. A deregistered instance simply stops being polled (doUpdateStatus returns early if !instance.isRegistered()), so eviction would never fire there.
I'll investigate a little further and probably will either
(a) keep a lightweight event subscription but reuse an existing handler/scheduler, or
(b) accept the small overhead since cleanup-on-deregister is the established pattern in SBA
There was a problem hiding this comment.
StatusUpdater polls the /health endpoint — it does not receive InstanceDeregisteredEvent.
Indeed. For a moment I had the impression it's handled there because the journal view shows all of them.
I'll investigate a little further and probably will either
(a) keep a lightweight event subscription but reuse an existing handler/scheduler, or
(b) accept the small overhead since cleanup-on-deregister is the established pattern in SBA
Wouldn't be enough to have a Spring Boot's @EventListener for the specific application event and remove the entry from the cache when it's received?
|
|
||
| describe('SSE reactive updates', () => { | ||
| it('should call fetchHealth once on mount, not on SSE version changes', async () => { | ||
| it('should call fetchCachedHealthGroups once on mount, not on SSE version changes', async () => { |
There was a problem hiding this comment.
Actually I think we should call the fetchHealthGroups when an SSE is received now, otherwise stale data may be potentially used.
This comment is not specifically related to this test case, but it's a more general statement.
There was a problem hiding this comment.
Good catch, and agreed in principle. The current else branch only invalidates the per-group details (group.data = null) but keeps the previously fetched group list, so a group added/removed at runtime would indeed show stale entries until the instance id changes or the view is remounted (although this seems to be an edge case).
I'll update onInstanceChanged so the same-instance branch also calls fetchHealthGroups() (which already resets healthGroupOpenStatus/healthGroupLoadingMap and clears stale data), making the displayed group list self-correcting on SSE updates.
There was a problem hiding this comment.
Yep.
Also because, from @SteKoe in a previous PR about a Thymeleaf fix, I discovered that the configuration properties can be changed at runtime from SBA.
Probably, to cover this case as well, we would need SSE also for the health groups, but I don't know how much worthy it really is. Unless a STATUS_CHANGED even is also created when the health response body changes in general and not just by the status (UP, DOWN and so on).
Performance and code clearity wise, it would then indeed be better to update the model of the instance to also have the health groups in it.
There was a problem hiding this comment.
Ok, I double-checked the code in StatusUpdater and, if I'm not mistaken, it's pretty dumb in the sense that it does no check with the previous state but just pushes the new event.
As such, indeed re-fetching the groups on every instance change should be enough even to cover runtime config updates done over the SBA UI.
…in/server/services/StatusUpdater.java Co-authored-by: Cosimo Damiano Prete <8491864+cdprete@users.noreply.github.com>
| this.cache.remove(instanceId); | ||
| } | ||
| else { | ||
| this.cache.put(instanceId, List.copyOf(groups)); |
There was a problem hiding this comment.
I think we should ensure that groups are unique, either here by using a LinkedHashSet instead of a List or in the UI to avoid potentially duplicated entries.
Closes #5352:
Added caching and a dedicated REST endpoint for health groups.
When SBA polls an application's /health endpoint, the response may include a groups field (e.g., ["liveness", "readiness"]). Previously this data was fetched on-the-fly from the client. Now:
Key design decisions