I agree, I would definitely not force implementors to provide the
method HealthCheck.getMetadata() - I believe it's rarely a good idea to
provide meta data programmatically (if we really wanted to have a unique
ID, we could add it as service properties like the name or the tags).
Additionally, forcing an implementor to define an id for a health check
would feel awkward to 99% of the people, as they only provide them but
never use it because the hypothetical signature
HealthCheckExecutor.execute(hcId) would normally only be used by JMX.
-Georg
Am 24.12.2013 12:11, schrieb Carsten Ziegeler:
It's not just about the metadata - it's about uniquely identifying a
HC
within the executor - the service object is not a good key,
especially as
we have no way to discard it from the cache. The service reference or
the
service id contained within the reference is a good key. And uniquely
identifying (for long running hcs) and caching the hc result is for
me the
number one reason for the executor service.
We should not change the HC interface as this is an incompatible
change and
would break all existing implementations - as we've discussed during
defining the first version of the API, there is no real need to have
the
name, tags etc. in the HC interface. Services executing an HC get the
service anyway from the service registry together with these
registration
properties and it makes the implementation unnecessarily complicated.
So we need something to uniquely identify a health check and as all
metadata is optional we don't have anything to be used. Now, as I
wrote in
the issue, I'm fine with having an execute(String name) method which
tries
to execute a HC with the given name. This would require that a HC
which can
be executed by this service either has a tag or a name.
On the other hand, if we go with ServiceReference we can execute all
HCs
through this service. And as Georg has pointed out, there are not
many
clients of this service anyway: its the jmx implementation and the
web
console.
Carsten
2013/12/24 Bertrand Delacretaz <bdelacre...@apache.org>
Hi,
About SLING-3278 - IMO execute(ServiceReference) at [1] is an
unnecessary leak of implementation details, it makes no sense at the
API level. It's HealthChecks that we are executing, not service
references.
IIUC the root cause is that the HealthCheck API doesn't provide
metadata that it should: tags, name, optional JMX MBean name, unique
ID maybe, all these things clearly belong to a HealthCheck, so the
API
should provide access to them, even if they are defined by service
properties.
I suggest adding a HealthCheckMetadata interface to provide those
things (tags, name, optional things in a Map maybe), and add a
HealthCheck.getMetadata() method that returns this. That'll break
existing HealthCheck implementations, which we indicate by bumping
up
the package version number, as this quite a new API it's better to
fix
it now than later.
-Bertrand
[1]
https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/healthcheck/core/src/main/java/org/apache/sling/hc/api/HealthCheckExecutor.java
