Skip to content

Feature background checks #25

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

Closed

Conversation

mschneider82
Copy link
Contributor

Added Start(interbval time.Duration) and Stop() to CompositeChecker
Changed Check() to reply last background check result if background checking was enabled

@mschneider82 mschneider82 mentioned this pull request Oct 18, 2019
@dimiro1
Copy link
Owner

dimiro1 commented Oct 19, 2019

I understand that such optimizations are required when the consumer of the checker (let's say Kubernetes) is too often checking the health of your application.

Is there any restriction that avoids the consumer to increase the frequency in which checks the health of the application?

@mschneider82 @Freakin.

@mschneider82
Copy link
Contributor Author

Yes, an abuse of too many requests to a non caching synchronous health handler could lead to too many database connections.
With my change its the library users choise, since calling Start() is optional. I may will add a return value to Start() something like an event channel, so the library user gets a notify if something is strange. What do you think?

@dimiro1
Copy link
Owner

dimiro1 commented Oct 20, 2019

@mschneider82 I think the implementation is unnecessarily adding complexity to the Composite checker. I think that it is better implemented using a decorator pattern on top of the Checker interface where you wrap another Checker.

Something like this:

type cacheChecker {
    wrappedChecker Checker
}

func (c *cacheChecker) Check() Health {
// call c.wrappedChecker.Check()  if not in cache
// return the health information
}

This is not really a problem I face myself and I think this is not a problem most people have. I believe this library must be used in a safe environment in which the application owner has some sort of control.

@mschneider82
Copy link
Contributor Author

mschneider82 commented Oct 21, 2019

What do you think when we add this type CacheChecker to the library?

Unfortunately we have to change the API for NewHandler() and add a Checker interface, to enable the possibility to create a Cached HTTP Handler

func NewHandler(checker Checker) Handler

or create a second NewCachedHandler(c Checker) function

@mschneider82
Copy link
Contributor Author

close this because #26 seems to be a better solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants