Skip to content
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

Merged
merged 6 commits into from
Jul 18, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,18 @@
* SPDX-License-Identifier: Apache-2.0
*
*/

package org.eclipse.microprofile.health;

import java.util.Map;
import java.util.Optional;

/**
* The health procedure response
* The health check procedure interface.
* Invoked by consumers to verify the healthiness of a computing node.
* Unhealthy nodes are expected to be terminated.
*
* @author Heiko Braun
* @since 13.06.17
*/
public interface HealthStatus {

enum State { UP, DOWN }

String getName();

State getState();

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

@FunctionalInterface
public interface HealthCheck {

Response call();
}
138 changes: 0 additions & 138 deletions api/src/main/java/org/eclipse/microprofile/health/HealthResponse.java

This file was deleted.

138 changes: 138 additions & 0 deletions api/src/main/java/org/eclipse/microprofile/health/Response.java
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();
Copy link
Contributor

@jmesnil jmesnil Jul 7, 2017

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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 in Response, but can be removed from ResponseBuilder, no?

Copy link
Contributor

@jmesnil jmesnil Jul 7, 2017

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

Copy link
Contributor Author

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.

Copy link
Contributor

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)


public abstract State getState();

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

private static <T> T find(Class<T> service) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

@heiko-braun heiko-braun Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmesnil done

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);


Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -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();

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
Loading