-
Notifications
You must be signed in to change notification settings - Fork 62
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
Revamp API and SPI #32
Changes from all commits
5a42399
8fd5bdd
c5e927a
1b27e7d
ebdfa92
0d0aeb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,138 @@ | ||
/* | ||
* Copyright (c) 2017 Contributors to the Eclipse Foundation | ||
* | ||
* See the NOTICES file(s) distributed with this work for additional | ||
* information regarding copyright ownership. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* You may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
*/ | ||
|
||
package org.eclipse.microprofile.health; | ||
|
||
import org.eclipse.microprofile.health.spi.SPIFactory; | ||
|
||
import java.security.AccessController; | ||
import java.security.PrivilegedAction; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.ServiceLoader; | ||
import java.util.logging.Level; | ||
import java.util.logging.Logger; | ||
|
||
/** | ||
* The response to a health check invocation. | ||
* <p> | ||
* The Response class is reserved for an extension by implementation providers. | ||
* An application should use one of the static methods to create a Response instance using a ResponseBuilder. | ||
* </p> | ||
* | ||
*/ | ||
public abstract class Response { | ||
|
||
private static final Logger LOGGER = Logger.getLogger(Response.class.getName()); | ||
|
||
private static volatile SPIFactory factory = null; | ||
|
||
public static ResponseBuilder named(String name) { | ||
|
||
if (factory == null) { | ||
synchronized (Response.class) { | ||
if (factory != null) { | ||
return factory.createResponseBuilder(); | ||
} | ||
|
||
SPIFactory newInstance = find(SPIFactory.class); | ||
|
||
if (newInstance == null) { | ||
throw new IllegalStateException("No SPIFactory implementation found!"); | ||
} | ||
|
||
factory = newInstance; | ||
} | ||
} | ||
|
||
return factory.createResponseBuilder().name(name); | ||
} | ||
|
||
// the actual contract | ||
|
||
public enum State { UP, DOWN } | ||
|
||
public abstract String getName(); | ||
|
||
public abstract State getState(); | ||
|
||
public abstract Optional<Map<String, Object>> getAttributes(); | ||
|
||
private static <T> T find(Class<T> service) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. implementation detail but the SPIFactory should be cached instead of loading it from the ServiceLoader every time a ResponseBuilder is invoked. It's reasonable to use the same SPIFactory for the whole lifetime of the application There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, we will improve this when moving forward with this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jmesnil done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name SPIFactory is not good. ResponseResolver is a better name for it and it follows the similar name convention as config. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need one more method setInstance so that it works in the OSGi environment. See ConfigProviderResolver.java. |
||
|
||
T serviceInstance = find(service, Response.getContextClassLoader()); | ||
|
||
// alternate classloader | ||
if(null==serviceInstance) { | ||
serviceInstance = find(service, Response.class.getClassLoader()); | ||
} | ||
|
||
// service cannot be found | ||
if(null==serviceInstance) { | ||
throw new IllegalStateException("Unable to find service " + service.getName()); | ||
} | ||
|
||
return serviceInstance; | ||
} | ||
|
||
private static <T> T find(Class<T> service, ClassLoader cl) { | ||
|
||
T serviceInstance = null; | ||
|
||
try { | ||
ServiceLoader<T> services = ServiceLoader.load(service, cl); | ||
|
||
for (T spi : services) { | ||
if (serviceInstance != null) { | ||
throw new IllegalStateException( | ||
"Multiple service implementations found: " | ||
+ spi.getClass().getName() + " and " | ||
+ serviceInstance.getClass().getName()); | ||
} | ||
serviceInstance = spi; | ||
} | ||
} | ||
catch (Throwable t) { | ||
LOGGER.log(Level.SEVERE, "Error loading service " + service.getName() + ".", t); | ||
} | ||
|
||
return serviceInstance; | ||
} | ||
|
||
|
||
|
||
private static ClassLoader getContextClassLoader() { | ||
return AccessController.doPrivileged((PrivilegedAction<ClassLoader>) () -> { | ||
ClassLoader cl = null; | ||
try { | ||
cl = Thread.currentThread().getContextClassLoader(); | ||
} | ||
catch (SecurityException ex) { | ||
LOGGER.log( | ||
Level.WARNING, | ||
"Unable to get context classloader instance.", | ||
ex); | ||
} | ||
return cl; | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* Copyright (c) 2017 Contributors to the Eclipse Foundation | ||
* | ||
* See the NOTICES file(s) distributed with this work for additional | ||
* information regarding copyright ownership. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* You may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
*/ | ||
|
||
package org.eclipse.microprofile.health; | ||
|
||
/** | ||
* A builder to construct a health procedure response. | ||
* | ||
* <p> | ||
* The ResponseBuilder class is reserved for an extension by implementation providers. | ||
* </p> | ||
*/ | ||
public abstract class ResponseBuilder { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be good to have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, once this is accepted (merged) we should add the changes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jmesnil done |
||
|
||
public abstract ResponseBuilder name(String name); | ||
|
||
public abstract ResponseBuilder withAttribute(String key, String value); | ||
|
||
public abstract ResponseBuilder withAttribute(String key, long value); | ||
|
||
public abstract ResponseBuilder withAttribute(String key, boolean value); | ||
|
||
public abstract Response up(); | ||
|
||
public abstract Response down(); | ||
|
||
public abstract Response state(boolean up); | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need a build() method to create a response object. Delete all the up, down and state, which should belong to Response.java. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Emily-Jiang let's address this in a separate PR |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,14 +19,16 @@ | |
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
*/ | ||
package org.eclipse.microprofile.health; | ||
|
||
package org.eclipse.microprofile.health.spi; | ||
|
||
import org.eclipse.microprofile.health.ResponseBuilder; | ||
|
||
/** | ||
* The basic health check procedure interface | ||
* | ||
* @author Heiko Braun | ||
* @since 13.06.17 | ||
* Created by hbraun on 07.07.17. | ||
*/ | ||
public interface HealthCheckProcedure { | ||
HealthStatus perform(); | ||
public interface SPIFactory { | ||
|
||
ResponseBuilder createResponseBuilder(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the structure is confusing. You need Response, Response Builder and ResponseResolver. ResponseResolver is to use service loader pattern to load an implementation of ResponseResolver. Response should just be a clean object to contain name, attributes, up, down, state etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Emily-Jiang let's address this in a separate PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @heiko-braun This whole PR is misleading. The structure is incorrect in my view. I would rather redo this PR instead of moving to a different PR. You can close this PR but not commit it and start a new one if you like. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's your point of view @Emily-Jiang. However @jmesnil and I think differently and hence I've decided to merge this PR. Feel free to submit proposals based on the the new API and we take it from there. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that the name of the health check belongs to the
Response
.I don't see an use case where a health check would return different names when it is called.
This may require a separate issue to discuss this but I think the
name
of the health check should be set on theHealthCheck
. In the design document, I proposed to introduce a@Health(name)
annotation to name aHealthCheck
(and remove the info from theResponse
).Please note that it's not clear to me what a name of a health check implies.
As far as I can tell, nothing in the spec or the API mandates uniqueness of names.
But the HTTP protocol document implies otherwise:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, it is part of the
Response
payload. How it's get's there is a different question. I like your proposal for@Health(name)
annotationsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following my above response: i think
name
needs to stay inResponse
, but can be removed fromResponseBuilder
, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After having written #34, I'm almost making my mind that is is up to the API provider to "name" the procedures when it is returning its payload.
I'm not sure we need to add the
name
to theResponse
though.The provide MUST name the health check in the HTTP payload but it is its responsibility to give it an unique name (base on my proposed
@Health
annotation and/or using unique generated IDs to take into account multiple deployments).Implementation-wise it seems to be trickier to add the name to the Response from the ResponseBuilder instead of waiting to create the HTTP payload to do that.
At that point, I have access to all health check procedures and I can ensure uniqueness among their names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest we move this to a separate thread (issue), wdyt? I'd prefer to consolidate the API first and then enhance it, a piece meal approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to move it to another issue (#34 can be used for this)