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

Asynchronous health checking #609

Closed
tomfitzhenry opened this Issue Jul 1, 2014 · 9 comments

Comments

Projects
None yet
5 participants
@tomfitzhenry

tomfitzhenry commented Jul 1, 2014

Problem: I'd like my calls to /healthcheck to execute quickly (say, <50ms), but some of my health checks are slow (say, 500ms).

Potential solution: Execute the health check every minute, using a ScheduledThreadPoolExecutor or some such, store the result, and have check() return the latest health checks results (checking that they're not stale too).

We can achieve this with either:

  1. Smart HealthCheck instances: Clients extend HealthCheck, creating a new method, checkSync(), which they execute every minute, and define check() to return the latest result of checkSync(). (Making sure the result isn't stale)
  2. Smart HealthCheckRegistry: clients submit HealthCheck instances to a AsyncHealthCheckRegistry or some such.

(2)'s API would potentially look like: AsyncHealthCheckRegistry#submit(mySlowCheck, 2, TimeUnit.Minutes), to have mySlowCheck executed once every two minutes.

Thoughts?

@tomfitzhenry

This comment has been minimized.

tomfitzhenry commented Jul 1, 2014

Workaround for clients: have two instances of HealthCheckRegistry:

  • one for quick tests, served at /healthcheck/quick
  • one for slow tests, served at /healthcheck/slow

@ryantenney ryantenney modified the milestones: 4.0.0, Future Aug 9, 2014

@ryantenney ryantenney modified the milestones: Future, 4.0.0 Aug 17, 2014

@tomfitzhenry

This comment has been minimized.

tomfitzhenry commented Jul 1, 2016

Two years to the day... freaky.

arkigabor added a commit to arkigabor/metrics that referenced this issue Feb 17, 2017

arkigabor added a commit to arkigabor/metrics that referenced this issue Feb 17, 2017

arkigabor added a commit to arkigabor/metrics that referenced this issue Feb 18, 2017

arkigabor added a commit to arkigabor/metrics that referenced this issue Feb 18, 2017

arteam added a commit that referenced this issue Feb 24, 2017

@stockmaj

This comment has been minimized.

Contributor

stockmaj commented Apr 24, 2017

The change for this introduced a dependency on JRE 1.7 (XXX in the date format string). Nothing in the documentation says JRE 1.7 is required, is there any reason this can't be changed back to Z for backwards compatibility?

@arteam

This comment has been minimized.

Member

arteam commented Apr 24, 2017

I guess you meant​ to comment on #1077, right? I think it's possible to change the format to be compatible with 1.6, if it doesn't change behaviour for 1.7 and 1.8.

@ryantenney

This comment has been minimized.

Member

ryantenney commented Apr 24, 2017

Great catch. The issue seems to be that XXX formats the offset as "-05:00", while Z formats as "-0500", which while still ISO8601 compliant, is not RFC3339 compliant.

@stockmaj

This comment has been minimized.

Contributor

stockmaj commented Apr 25, 2017

I can't tell from your answer if it's OK to change it or not. If 3.2 intentionally requires 1.7, I think it should say that in the Release Notes. If not, this needs to be changed, one way or another.

@stockmaj

This comment has been minimized.

Contributor

stockmaj commented Apr 25, 2017

@arteam When I looked at the history it looked like it came in on #609, but I do see it on #1077 also. I don't use github a lot. The reason I'm just asking and not submitting a pull is that I would have to do the pull from home, not from work. If I'm going to do that, I'd like to work out the details ahead.

@arteam

This comment has been minimized.

Member

arteam commented Apr 27, 2017

I think it would be correct to change the pattern, because we still want to maintain runtime compatibility with JDK 1.6.

@arkigabor

This comment has been minimized.

Contributor

arkigabor commented Jul 19, 2017

Sorry about this. This was an oversight from my side, it was meant to be Java 6 compatible.

@arteam arteam closed this Aug 24, 2017

@arteam arteam removed this from the 4.0.0 milestone Dec 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment