Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #18540: Healthcheck checks happen to often #3354

Conversation

ElaadF
Copy link
Member

@ElaadF ElaadF commented Nov 10, 2020

@ElaadF ElaadF requested a review from fanf November 10, 2020 10:29
ApplicationLogger.info("Property 'metrics.healthcheck.scheduler.period' is missing or empty in rudder.configFile. Default to 5mins.")
5.minutes
ApplicationLogger.info("Property 'metrics.healthcheck.scheduler.period' is missing or empty in rudder.configFile. Default to 6 hours.")
20.seconds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 hours ?

@@ -50,10 +50,10 @@ class HealthcheckNotificationService(
v <- Ref.make[List[HealthcheckResult]](List.empty)
} yield v).runNow

private[this] def reloadCache(ref: Ref[List[HealthcheckResult]]): UIO[Unit] = {
def reloadCache(ref: Ref[List[HealthcheckResult]] = healthcheckCache): UIO[Unit] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why that change?
Default parameters are a code smell, they tend to bring a lot of complicated problem on the long term.
If you need a way to say "now, do init your internal things", it's better to have a dedicated init method. That way, it's also easier to understand you class state machine (entry points, transition, etc).

@@ -536,6 +536,9 @@ class Boot extends Loggable {
}
}

// Run a health check
RudderConfig.healthcheckNotificationService.reloadCache()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer a startService here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a duplicated healthcheckNotificationService used only at boot ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, just the method: RudderConfig.healthcheckNotificationService.startService. Or start or init

@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit bf36232 into Normation:branches/rudder/6.2 Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants