fix(17180): EventCreator ignores HealthMonitor update when squelching enabled

[mxtartaglia-sl]

Description:

When squelched, the EventCreator, if previously registered an unhealthy status by the health monitor, ignores the message that informs it that things are now in a healthy state again. Thus, it fails to start creating events again, given that the monitor doesn't repeat reports if they are unchanged.

After this change, the monitor tracks the transitions to healthy and reports the healthy state repeatedly (even if unchanged) every 1 second.

Related issue(s):

Fixes #17180

Notes for reviewer:

Checklist

• Documented (Code comments, README, etc.)
• Tested (unit, integration, etc.)

[cody-littley]

This change introduces a race condition. The following conditions must be met in order to safely flush the wiring framework:

• no new data may be entering the system
• all cycles in the wiring graph must be broken. This is done by squelching.
• data flowing within the wiring diagram is now a DAG. Flush components in topological order.

I'm not sure if squelching is going to cause a bug since didn't do a deep dive on the reported issue. But I'm fairly confident that removing it is going to open the door for different bugs.

The complex workflow the system currently uses is in a large part due to the fact that it reuses things after a reconnect. It would be WAY simpler and safer if things were simply discarded and reconstructed after a reconnect.

[mxtartaglia-sl]

Hey @cody-littley, thanks for the insight. Truly appreciated.

background: Oleg's analysis exposed a situation where, after a reconnect, when squelching is enabled, the EventCreator, if previously registered from the health monitor an unhealthy duration, would ignore all incoming inputs, even the one that, from the health monitor, would inform that the platform is in a healthy status again, failing to start creating events once more.

I'm not sure if squelching is going to cause a bug since I didn't do a deep dive on the reported issue. But I'm fairly confident that removing it is going to open the door for different bugs.

I see your point; I think that could happen if we completely remove the squelching feature (as is happening here PR).

I am removing the squelching ONLY from the EventCreator in the current PR.
The rationale is:
a) the eventcreator #maybeCreateEvent wire is already not emitting new events, given the platform status.
b) eventcreator # "health info" wire must accept input regardless of a reconnect being processed.
c) eventcreator # "PlatformStatus" wire comes from the statusStateMachine, which was currently flushed before we used to enable squelching on the eventcreator component, so it would be ok to register the new input.
d) eventcreator # "event window" is replaced in ReconnectStateLoader#loadReconnectState()
e) whatever happens with eventcreator # "PlatformEvent" is later cleared using the "clear" wire.

The drawback is that it relies too much on how things work today.

It would be WAY simpler and safer if things were simply discarded and reconstructed after a reconnect.

Agreed, although we are far from that to be possible, other options that appeared are:

• The health monitor would report the duration even if it is unchanged, ensuring that the event creator would eventually receive the input when it stops squelching.
• Squelching is configurable per wire instead of per taskscheduler.

Do you think I'm missing something? Eager to hear back from you...

[netopyr]

I agree with Cody. Even if the fix would solve the observed issue, it would introduce much risk. PlatformCoordinator.clear() could never be used in any other scenario. Can we ensure this is the case and always will be?

[mxtartaglia-sl]

@netopyr PlatformCoordinator#clear javadoc states:

    /**
     * Safely clears the system in preparation for reconnect. After this method is called, there should be no work
     * sitting in any of the wiring queues, and all internal data structures within wiring components that need to be
     * cleared to prepare for a reconnect should be cleared.
     */

I wouldn't say it was initially intended for any other use.
That said, I'm all up for not introducing the indirect coupling and leaving the squelching (at least, if really not necessary) as documentation of the relationship. @lpetrovic05 any thoughts?

@netopyr, which of the alternatives would you prefer, if not this one?

A 3rd option proposed by Oleg is to have a different type of wire that would enable a direct modification of component's internal state from other tasksSchedulers belonging to connected components.

[lpetrovic05]

@cody-littley @mxtartaglia-sl @netopyr
My thought is this. What Cody said about reconstructing is definitely the best and simplest approach, and this is what we plan to do. But in the meantime, we should probably fix this bug. This to me seems the simplest and safest approach to fixing this issue until we get around to rebuilding everything.

[cody-littley]

If I understand the issue correctly, the squelched component is accidentally squelching the message that informs it that things are now in a healthy state again?

If that's the case, a simple hack that wouldn't introduce new bugs would be to have the health monitor periodically send an "everything is good" message... perhaps once a second or so. (Note, this message probably shouldn't be sent while the node is reconnecting... don't want to clog up queues.) Currently, it only sends that message when the status changes, but it wouldn't hurt anything to send it more frequently.

[mxtartaglia-sl]

@cody-littley correct, that is the issue.

[lpetrovic05]

If I understand the issue correctly, the squelched component is accidentally squelching the message that informs it that things are now in a healthy state again?

If that's the case, a simple hack that wouldn't introduce new bugs would be to have the health monitor periodically send an "everything is good" message... perhaps once a second or so. (Note, this message probably shouldn't be sent while the node is reconnecting... don't want to clog up queues.) Currently, it only sends that message when the status changes, but it wouldn't hurt anything to send it more frequently.

We considered this approach, but we concluded that it will cause a lot more tasks to be created, which might have a performance impact. Because of this, we thought the current approach would be safer

[cody-littley]

a lot more tasks to be created

It depends on the frequency. There are currently thousands, if not tens of thousands, of tasks flowing through the wiring framework each second. A few extra every second or two is unlikely to produce a measurable delta.

[edward-swirldslabs]

It depends on the frequency. There are currently thousands, if not tens of thousands, of tasks flowing through the wiring framework each second. A few extra every second or two is unlikely to produce a measurable delta.

I agree with this.

[lpetrovic05]

It depends on the frequency. There are currently thousands, if not tens of thousands, of tasks flowing through the wiring framework each second. A few extra every second or two is unlikely to produce a measurable delta.

I agree with this.

Currently, the check is done 1000 times per second, there are 3 wires bound to its output. This is at least an additional 3000 tasks per second, I don't think this is insignificant.

@OlegMazurov @poulok Thoughts on this?

[edward-swirldslabs]

It depends on the frequency. There are currently thousands, if not tens of thousands, of tasks flowing through the wiring framework each second. A few extra every second or two is unlikely to produce a measurable delta.

I agree with this.

Currently, the check is done 1000 times per second, there are 3 wires bound to its output. This is at least an additional 3000 tasks per second, I don't think this is insignificant.

@OlegMazurov @poulok Thoughts on this?

Is a task something that is generating an item on a queue that has to be handled, or java byte code executing to do a branch test?

[mxtartaglia-sl]

Is a task something that is generating an item on a queue that has to be handled

Depends on the scheduler type, but for most used cases, yes.

[lpetrovic05]

It depends on the frequency. There are currently thousands, if not tens of thousands, of tasks flowing through the wiring framework each second. A few extra every second or two is unlikely to produce a measurable delta.

I agree with this.

Currently, the check is done 1000 times per second, there are 3 wires bound to its output. This is at least an additional 3000 tasks per second, I don't think this is insignificant.
@OlegMazurov @poulok Thoughts on this?

Is a task something that is generating an item on a queue that has to be handled, or java byte code executing to do a branch test?

It is an instance that is sent to the ForkJoinPool to be executed asynchronously. Its sort of like a queue, but not exactly.

[OlegMazurov]

Currently, the check is done 1000 times per second, there are 3 wires bound to its output. This is at least an additional 3000 tasks per second, I don't think this is insignificant.

I believe we can afford that as a tentative solution (we have resources available even at high TPS).

[mxtartaglia-sl]

we are going to move forward with this approach:

have the health monitor periodically send an "everything is good" message... perhaps once a second or so.

[timo0]

should we have this configurable?