Changes for 154 #155
Changes for 154 #155
Conversation
| if (healthcheckActive.get()) { | ||
| return check(SLIDING_WINDOW_SIZE, ALPHA); | ||
| } else { | ||
| logger.warn("Queue HealthCheck Disabled, Returning HealthyResult"); |
There was a problem hiding this comment.
I don't think we should have a warn for this - it will result in a lot of warn that do not need any actions. Either remove it completely or keep it in DEBUG
| @@ -71,4 +71,9 @@ public interface KillbillServerConfig extends KillbillPlatformConfig { | |||
| @Default("0s") | |||
| @Description("Shutdown delay before starting shutdown sequence") | |||
| public TimeSpan getShutdownDelay(); | |||
|
|
|||
| @Config(KILL_BILL_NAMESPACE + "server.queue.healthcheck") | |||
There was a problem hiding this comment.
Could we make the property server.queue.healthcheck.enabled to be clear - unless we have other instances of those that follow this specific pattern?
Also, as a subsequent task, can you create doc PR to add this property?
There was a problem hiding this comment.
Could we make the property server.queue.healthcheck.enabled to be clear - unless we have other instances of those that follow this specific pattern?
Yes, I was following the pattern for the other properties in KillBillServerConfig like server.test.mode, server.test.clock.redis. I agree, having the enabled keyword in the property name makes it clearer. Let me know if you would like me to still go ahead with the change.
Also, as a subsequent task, can you create doc PR to add this property?
Noted, will do.
| if (healthy || !healthcheckActive.get()) { | ||
| logger.info("System is healthy or healthcheck is disabled, Returning HealthyResult"); |
There was a problem hiding this comment.
I would remove both traces or make them DEBUG: This is potentially called every 5 seconds.
There was a problem hiding this comment.
Fixed, I had added these statements to help me during testing, but I guess I forgot to delete them. Thanks for pointing this out!
Changes for #154