Healthchecks plugin init first#9264
Conversation
|
@LukaszRozmej @asdacap concerning #9131 (review), We need to initialise HealthCheckPlugin always first, so Autofac can override properly from any other modules with the rule of "last initialised wins". The other option would be to do something like that in But that's even worse because it depends on the module's alphabetical order. Please let me and @damian-orzechowski know your thoughts! |
|
What thing do healthcheckplugin need to override? |
HealthCheckPlugin shouldn't override something, but it should be overriden. We shouldn't have CL health checks in arbitrum like that NethermindEth/nethermind-arbitrum#206 My initial approach was this one #9131 |
|
So... what is the problem with NethermindEth/nethermind-arbitrum#206? |
The problem is that by default Autofac's rule is to keep the latest initialisation and ArbitrumPlugin is initialized before HealthChecksPlugin because of Alphabetical order so override doesn't work. |
|
Ahh... okok. Make sense. Can you move the |
Sure, the question is more of architectural approach, whether we prefer this change in priority over this approach #9131 (It has merge conflicts right now but you can see the point) |
| if (f.Name == "HealthChecksPlugin") return -1; | ||
| if (s.Name == "HealthChecksPlugin") return 1; |
There was a problem hiding this comment.
Why not put it ahead in PluginOrder?
There was a problem hiding this comment.
Since we all feel like that this approach is better, I'll make it work through priority - thanks!
There was a problem hiding this comment.
@LukaszRozmej ArbitrumPlugin is IConsensusPlugin so it always goes at front
|
I've made some refactoring to keep the old functionallity + HealthCheck at the beginning + Priority field but I still don't like it I think we need to simplify it even more. Tests are also commented out only for now. |
| if (p.GetType().Name == "HealthChecksPlugin") | ||
| return -2000; |
There was a problem hiding this comment.
Because if we want to keep the rule consensus first as number one, healthchecks need somehow to overrule that 😞 That's why I said maybe we need to simplify things here at standup
|
Replaced by #9308 |
Initialise Health Checks Plugin first, in order to be able to always override properly by other modules like Arbitrum, regardless of the alphabetical order.
Previous discussion on this PR #9131