Revamp API and SPI #32

heiko-braun · 2017-07-07T06:45:15Z

Refactor builder builder pattern
Introduce SPI delegation (using service loader)
Change API terms

- Introduce SPI delegation (using service loader) - Change API terms

heiko-braun · 2017-07-07T06:49:57Z

The PR introduces a revamped API and that I would like to use as a baseline moving forward. It uses more descriptive API terms and introduces a contract between the implementing runtime and health API using the ResponseBuilder and Response classes.

Implementors are expected to provide an implementation of SPIFactory that is used to load a ResponseBuilder implementation. From a users point of view this will become the main entry point (see Response.named()) to construct Response implementations that carry the health check response payload.

Let me know what you think.

jmesnil · 2017-07-07T09:38:52Z

api/src/main/java/org/eclipse/microprofile/health/Response.java

+
+ public abstract Optional<Map<String, Object>> getAttributes();
+
+ private static <T> T find(Class<T> service) {


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

agreed, we will improve this when moving forward with this PR

@jmesnil done

The name SPIFactory is not good. ResponseResolver is a better name for it and it follows the similar name convention as config.

You need one more method setInstance so that it works in the OSGi environment. See ConfigProviderResolver.java.

jmesnil · 2017-07-07T09:40:51Z

api/src/main/java/org/eclipse/microprofile/health/Response.java

+ try {
+ Iterator<T> iterator = ServiceLoader.load(service, Response.class.getClassLoader()).iterator();
+
+ if (iterator.hasNext()) {


Assuming that a correct configured application has a single SPIFactory, we should check for multiple instances instead of letting the latest one that is loaded win.
(Have a look at https:/eclipse/microprofile-config/blob/master/api/src/main/java/org/eclipse/microprofile/config/spi/ConfigProviderResolver.java#L134 for a similar use case)

@jmesnil Done

jmesnil · 2017-07-07T09:42:55Z

api/src/main/java/org/eclipse/microprofile/health/ResponseBuilder.java

+ * The ResponseBuilder class is reserved for an extension by implementation providers.
+ * </p>
+ */
+public abstract class ResponseBuilder {


It'd be good to have a Response state(boolean up) method too (as proposed in #23 for the previous API).

yes, once this is accepted (merged) we should add the changes

@jmesnil done

jmesnil · 2017-07-07T09:44:45Z

api/src/main/java/org/eclipse/microprofile/health/spi/SPIFactory.java

 */
-public interface HealthStatus {
+public interface SPIFactory {


Name is generic and non-descriptive. Maybe something like ResponseBuilderFactory instead?

It is intended to capture all possible SPI contracts. ResponseBuilder is currently the only one we have, but who knows?

If there is differents SPI contracts, I'd expect to load each through the ServiceLoader with their respective contract interface (instead of grouping them in a single SPIFactory class).
Especially is some contracts are optional and other mandatory

yes, that would work also.

jmesnil · 2017-07-07T09:53:05Z

api/src/main/java/org/eclipse/microprofile/health/Response.java

+
+ public enum State { UP, DOWN }
+
+ public abstract String getName();


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 the HealthCheck. In the design document, I proposed to introduce a @Health(name) annotation to name a HealthCheck (and remove the info from the Response).

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:

The JSON response MUST contain the id entry specifying the name of the check, to support protocols that support external identifier (i.e. URI)

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) annotations

following my above response: i think name needs to stay in Response, but can be removed from ResponseBuilder, no?

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 the Response 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.

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.

+1 to move it to another issue (#34 can be used for this)

heiko-braun · 2017-07-11T07:20:03Z

@jmesnil Do you see anything else that would hold back merging this PR?

@Emily-Jiang Any thoughts on this?

jmesnil

looks good to me

Emily-Jiang · 2017-07-11T08:13:58Z

api/src/main/java/org/eclipse/microprofile/health/Response.java

+
+ public abstract Optional<Map<String, Object>> getAttributes();
+
+ private static <T> T find(Class<T> service) {


The name SPIFactory is not good. ResponseResolver is a better name for it and it follows the similar name convention as config.

Emily-Jiang · 2017-07-11T08:17:35Z

api/src/main/java/org/eclipse/microprofile/health/Response.java

+
+ public abstract Optional<Map<String, Object>> getAttributes();
+
+ private static <T> T find(Class<T> service) {


You need one more method setInstance so that it works in the OSGi environment. See ConfigProviderResolver.java.

Emily-Jiang · 2017-07-11T08:19:07Z

api/src/main/java/org/eclipse/microprofile/health/ResponseBuilder.java

+
+ public abstract Response state(boolean up);
+
+


You need a build() method to create a response object. Delete all the up, down and state, which should belong to Response.java.

@Emily-Jiang let's address this in a separate PR

Emily-Jiang · 2017-07-11T08:22:57Z

api/src/main/java/org/eclipse/microprofile/health/spi/SPIFactory.java

+public interface SPIFactory {
+
+ ResponseBuilder createResponseBuilder();
+


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.

@Emily-Jiang let's address this in a separate PR

@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.

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.

heiko-braun added 2 commits July 7, 2017 08:08

- Refactor builder builder pattern

5a42399

- Introduce SPI delegation (using service loader) - Change API terms

Introduce SPIFactory

8fd5bdd

jmesnil reviewed Jul 7, 2017

View reviewed changes

heiko-braun added 3 commits July 7, 2017 13:43

Cache SPIFactory instances

c5e927a

Faile when multiple SPI implementations are discovered

1b27e7d

Added ResponseBuilder.state(boolean up), as suggested by @jmesnil

ebdfa92

heiko-braun mentioned this pull request Jul 7, 2017

Add HealthResponse.state(boolean) method #23

Closed

Turn HealthCheck into @FunctionalInterface

0d0aeb5

heiko-braun mentioned this pull request Jul 7, 2017

Flag HealthCheckProcedure as a @FunctionalInterface #25

Closed

jmesnil approved these changes Jul 11, 2017

View reviewed changes

Emily-Jiang requested changes Jul 11, 2017

View reviewed changes

heiko-braun mentioned this pull request Jul 17, 2017

Make HealthCheckProcedure a functional interface #38

Closed

heiko-braun merged commit 2fd6713 into eclipse:master Jul 18, 2017

jmesnil mentioned this pull request Aug 1, 2017

Revisit API terms #43

Closed

Pandrex247 mentioned this pull request Aug 6, 2017

Cater for OSGi Environments #44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp API and SPI #32

Revamp API and SPI #32

heiko-braun commented Jul 7, 2017

heiko-braun commented Jul 7, 2017

jmesnil Jul 7, 2017

heiko-braun Jul 7, 2017 •

edited

Loading

heiko-braun Jul 7, 2017

Emily-Jiang Jul 11, 2017

Emily-Jiang Jul 11, 2017

jmesnil Jul 7, 2017

heiko-braun Jul 7, 2017

jmesnil Jul 7, 2017

heiko-braun Jul 7, 2017

heiko-braun Jul 7, 2017

jmesnil Jul 7, 2017 •

edited

Loading

heiko-braun Jul 7, 2017 •

edited

Loading

jmesnil Jul 7, 2017

heiko-braun Jul 7, 2017

jmesnil Jul 7, 2017 •

edited

Loading

heiko-braun Jul 7, 2017

heiko-braun Jul 7, 2017

jmesnil Jul 7, 2017 •

edited

Loading

heiko-braun Jul 7, 2017

jmesnil Jul 7, 2017

heiko-braun commented Jul 11, 2017

jmesnil left a comment

Emily-Jiang Jul 11, 2017

Emily-Jiang Jul 11, 2017

Emily-Jiang Jul 11, 2017

heiko-braun Jul 18, 2017

Emily-Jiang Jul 11, 2017

heiko-braun Jul 18, 2017

Emily-Jiang Jul 18, 2017

heiko-braun Jul 18, 2017


		public abstract Optional<Map<String, Object>> getAttributes();

		private static <T> T find(Class<T> service) {


		public enum State { UP, DOWN }

		public abstract String getName();

		public interface SPIFactory {

		ResponseBuilder createResponseBuilder();

Revamp API and SPI #32

Revamp API and SPI #32

Conversation

heiko-braun commented Jul 7, 2017

heiko-braun commented Jul 7, 2017

Choose a reason for hiding this comment

heiko-braun Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmesnil Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

heiko-braun Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmesnil Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmesnil Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heiko-braun commented Jul 11, 2017

jmesnil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heiko-braun Jul 7, 2017 •

edited

Loading

jmesnil Jul 7, 2017 •

edited

Loading

heiko-braun Jul 7, 2017 •

edited

Loading

jmesnil Jul 7, 2017 •

edited

Loading

jmesnil Jul 7, 2017 •

edited

Loading