Hi Betrand and Christian,
now I'm afraid we might go a step backward... some things that you
suggest
to be unnecessary are essentially important from my point of view -
supported by one year experience getting to this stage :). Here are
my
explanations:
--- RE: executeXXX Methods
We definitely need the executeForTags(String... tags) and
executeAll()
methods because only this way we get parallel execution with
intelligent
timeout handling and current results:
- If all checks are quick (let's say the the longest check requires
100ms
so finish), executeXxx() will return after 100ms having all current
results
(Bertrand, your patch was returning the results of the last call, if
the
last call was 2h ago, all results would be two hours old!)
- If all checks except one are quick and let's say the one check
takes 3s
timing out, we get all current results for the quick checks and one
WARN
timeout result for one check (if we wanted we could change it to the
last
cached result, but for my last project this behaviour worked better,
also
see next bullet point)
- The global timeout is normally configured reasonably ensuring that
when
things are ok, checks never time out
- Sometimes, if things go bad, a check can be stuck completely (I
know
this shouldn't happen, but I have seen it happening). For this case
a
special timeout can be configured marking checks as critical if they
exceed
this timeout (10sec default)
The current implementation is robust and correct in many ways:
- no matter how many calls to the health check executor are made
(e.g. 100
per sec or only 1 every day), we get the latest results (respecting
the
cache TTL) but without ever executing the same check twice (two
request to
the web console started at the exact same time will only cause each
check
to be called once and the two requests will return the exact same
result)
- it uses never more threads than checks exist in the system
- no matter how bad one check is implemented or how "sick" the
aspect the
check observes is, it will never in any way cause the executor to
stop
working except that it returns red for the check (returning the
results for
the other checks correctly)
--- RE: Health Check MetaData
It is important to distinguish two aspects:
1) defining the meta data
2) allow access to the meta data
to 1) I definitely think this should be done in a declarative way
(not
programmatically by adding a method to the HealthCheck interface!).
There
is just no need for touring complete code here. Using the service
properties as it's done in the moment is IMHO the best way.
to 2) There should be definitely a way to access the meta data from
the
result. My way was to use the class HealthCheckDescriptor and add a
method
result.getHealthCheckDescriptor() to the result (funny enough that
class
was called HealthCheckMetaData in my first version but I renamed it
later
because I thought Descriptor is better). Having a flat result
structure was
requested by Carsten, that's why we have the methods
getHealthCheckName()
and getHealthCheckTags() now. The cleanest solution my opinion is to
go
back to result.getHealthCheckDescriptor() - or we can call it
getHealthCheckMetaData() if you prefer. Also we can create an
interface
HealthCheckMetaData() and keep that in line with the service
properties.
--- RE: "why can't we provide these values from the HealthCheck that
was
used to create it?" (these values=timing information)
Because
- every check would have to do it (code duplication, pushing
complexity to
the implementor)
- it cannot be guaranteed, that every health check does it correctly
--- RE: UnsupportedOperationException in Result
Using the UnsupportedOperationException is IMHO an elegant solution.
The
exception is part of the JDK and for instance the Collection API is
using
it as well. It is elegant because
- From a conceptual/object oriented view, it totally makes sense to
have
getElapsedTime() or getFinishedAt() as properties of a result.
However, the
HC itself cannot provide this information (see last section) and
it's fine
to throw UnsupportedOperationException for these results then.
- If the methods would be taken away from the interface, a cast to
ExecutionResult would be needed (that is hidden at the moment
anyway) to
show this information in the web console
- From an implementors point of view, it is quickly obvious that the
methods are only available when using the executor (the exception
message
is pretty clear)
- Typical implementors would not even notice as they just construct
the
result, but don't call methods on it (that is the job of the web
console
and JMX)
- We should have getElapsedTime() and getFinishedAt() in the
HealthCheckResult interface because this information is useful for
human
observers (and should be shown in web console / JMX)
-Georg
Am 30.12.2013 11:30, schrieb Carsten Ziegeler:
The more I think about it, the more I get the feeling that if we
can't
away
with execute(ServiceReference) in the executor api, this should be
the
only
method of that service.
The two others can easily be replaced by using the HCFilter and
then
calling the method above for each service reference. It's just one
line
more code, but this would completely remove the need to add meta
information about the HC in the result and make the whole
implementation
easier.
WDYT?
Carsten
2013/12/30 Carsten Ziegeler <[email protected]>
2013/12/27 Bertrand Delacretaz <[email protected]>
Hi Carsten and Georg,
I was going to say that I still disagree, but being outnumbered
I'd
let it go if it's just about a single method that I don't
like...and
then I reviewed the org.apache.sling.hc.api package once again
and
it's worse than I thought.
The problem is really, if we don't have a method with this
signature
how
can the jmx implementation make use of the executor while the
executor is
still able to cache the result, do async processing etc. ? I don't
see
any
other solution atm, apart from moving the jmx implementation into
the
core.
You guys both say you don't want metadata in APIs...and the
HealthCheckResult interface that you added has these two methods
(which make sense BTW):
String getHealthCheckName();
List<String> getHealthCheckTags();
So a HC is not brave enough to have this in its API, but a Result
needs them? This clearly demonstrates that we need a
HealthCheckMetadata interface that provides those values.
I think these are different concerns, a service(!) does not need
an api
to expose the metadata because it's already in the service
registration.
When we return a set of results, there need to be way of
correlation
between a single result and an actual check, otherwise you get a
bunch of
results without any additional information about the check.
The way out of this, would be to just have a single method
execute(ServiceReference) in the executor :) - but as soon as we
allow
execution of more than a single HC we need this additional
information.
On top of that, the Result throws UnsupportedException on these
methods...how ugly is that, why can't it provide these values
from the
HealthCheck that was used to create it?
I haven't looked deeply at this part yet...
To me this smells of implementation-driven APIs, we are used to
better
design in Sling.
As pointed out, I'm all ears for a better solution of the
executor
interface that allows us to get the above mentioned features. And
as
pointed out below, if we simplify the executor service to a single
method,
we don't need to make any changes to Result or add any additional
information.
Carsten
What do others think?
-Bertrand
[1]
https://svn.apache.org/repos/asf/sling/trunk/bundles/
extensions/healthcheck/core/src/main/java/org/apache/sling/hc/api
--
Carsten Ziegeler
[email protected]
