You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2016/02/17 01:31:43 UTC
[6/7] brooklyn-server git commit: address review comments on
https://github.com/apache/brooklyn-server/pull/14
address review comments on https://github.com/apache/brooklyn-server/pull/14
better javadoc and logging and cleaner signatures
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo
Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/e1e3764e
Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/e1e3764e
Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/e1e3764e
Branch: refs/heads/master
Commit: e1e3764e63730199b0030fc06352a4ad90fc84e6
Parents: 7b9f8c7
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Authored: Tue Feb 16 23:56:39 2016 +0000
Committer: Alex Heneveld <al...@cloudsoftcorp.com>
Committed: Tue Feb 16 23:56:39 2016 +0000
----------------------------------------------------------------------
.../location/jclouds/JcloudsLocation.java | 4 +-
.../rest/resources/ApplicationResource.java | 12 ++++--
.../rest/util/DefaultExceptionMapper.java | 14 +++----
.../InfrastructureDeploymentTestCaseTest.java | 4 +-
.../brooklyn/util/exceptions/Exceptions.java | 43 +++++++++++++-------
.../exceptions/RuntimeInterruptedException.java | 9 +++-
6 files changed, 54 insertions(+), 32 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e1e3764e/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
----------------------------------------------------------------------
diff --git a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
index 346e805..4a5cf7a 100644
--- a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
+++ b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
@@ -1500,7 +1500,7 @@ public class JcloudsLocation extends AbstractCloudMachineProvisioningLocation im
templateBuilder, this, config.getDescription()});
// Finally try to build the template
- Template template;
+ Template template = null;
Image image;
try {
template = templateBuilder.build();
@@ -1509,7 +1509,7 @@ public class JcloudsLocation extends AbstractCloudMachineProvisioningLocation im
LOG.debug("jclouds found template "+template+" (image "+image+") for provisioning in "+this+" for "+config.getDescription());
if (image==null) throw new NullPointerException("Template does not contain an image (templateBuilder.build returned invalid template)");
} catch (AuthorizationException e) {
- LOG.warn("Error resolving template: not authorized (rethrowing: "+e+")");
+ LOG.warn("Error resolving template -- not authorized (rethrowing: "+e+"); template is: "+template);
throw new IllegalStateException("Not authorized to access cloud "+this+"; check credentials", e);
} catch (Exception e) {
try {
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e1e3764e/rest/rest-server/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java
----------------------------------------------------------------------
diff --git a/rest/rest-server/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java b/rest/rest-server/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java
index a74ad66..d894aea 100644
--- a/rest/rest-server/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java
+++ b/rest/rest-server/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java
@@ -85,7 +85,6 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.google.common.base.Preconditions;
-import com.google.common.base.Throwables;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
@@ -239,7 +238,7 @@ public class ApplicationResource extends AbstractBrooklynRestResource implements
@Override
public Response createFromYaml(String yaml) {
// First of all, see if it's a URL
- Preconditions.checkNotNull(yaml, "Blueprint cannot be null");
+ Preconditions.checkNotNull(yaml, "Blueprint must not be null");
URI uri = null;
try {
String yamlUrl = yaml.trim();
@@ -267,8 +266,11 @@ public class ApplicationResource extends AbstractBrooklynRestResource implements
spec = createEntitySpecForApplication(yaml);
} catch (Exception e) {
Exceptions.propagateIfFatal(e);
- Throwable root = Throwables.getRootCause(e);
- if (root instanceof UserFacingException) throw (UserFacingException) root;
+ UserFacingException userFacing = Exceptions.getFirstThrowableOfType(e, UserFacingException.class);
+ if (userFacing!=null) {
+ log.debug("Throwing "+userFacing+", wrapped in "+e);
+ throw userFacing;
+ }
throw WebResourceUtils.badRequest(e, "Error in blueprint");
}
@@ -338,6 +340,8 @@ public class ApplicationResource extends AbstractBrooklynRestResource implements
try {
spec = createEntitySpecForApplication(potentialYaml);
} catch (Exception e) {
+ Exceptions.propagateIfFatal(e);
+
// TODO if not yaml/json - try ZIP, etc
throw WebResourceUtils.badRequest(e, "Error in blueprint");
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e1e3764e/rest/rest-server/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java
----------------------------------------------------------------------
diff --git a/rest/rest-server/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java b/rest/rest-server/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java
index d11149c..ef3eb32 100644
--- a/rest/rest-server/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java
+++ b/rest/rest-server/src/main/java/org/apache/brooklyn/rest/util/DefaultExceptionMapper.java
@@ -27,9 +27,6 @@ import javax.ws.rs.core.Response.Status;
import javax.ws.rs.ext.ExceptionMapper;
import javax.ws.rs.ext.Provider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-import org.yaml.snakeyaml.error.YAMLException;
import org.apache.brooklyn.core.mgmt.entitlement.Entitlements;
import org.apache.brooklyn.rest.domain.ApiError;
import org.apache.brooklyn.rest.domain.ApiError.Builder;
@@ -38,8 +35,9 @@ import org.apache.brooklyn.util.core.flags.ClassCoercionException;
import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.exceptions.UserFacingException;
import org.apache.brooklyn.util.text.Strings;
-
-import com.google.common.base.Throwables;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.yaml.snakeyaml.error.YAMLException;
@Provider
public class DefaultExceptionMapper implements ExceptionMapper<Throwable> {
@@ -95,9 +93,9 @@ public class DefaultExceptionMapper implements ExceptionMapper<Throwable> {
}
// Before saying server error, look for a user-facing exception anywhere in the hierarchy
- Throwable root = Throwables.getRootCause(throwable1);
- if (root instanceof UserFacingException) {
- return ApiError.of(root.getMessage()).asBadRequestResponseJson();
+ UserFacingException userFacing = Exceptions.getFirstThrowableOfType(throwable1, UserFacingException.class);
+ if (userFacing instanceof UserFacingException) {
+ return ApiError.of(userFacing.getMessage()).asBadRequestResponseJson();
}
Throwable throwable3 = Exceptions.collapse(throwable2);
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e1e3764e/test-framework/src/test/java/org/apache/brooklyn/test/framework/InfrastructureDeploymentTestCaseTest.java
----------------------------------------------------------------------
diff --git a/test-framework/src/test/java/org/apache/brooklyn/test/framework/InfrastructureDeploymentTestCaseTest.java b/test-framework/src/test/java/org/apache/brooklyn/test/framework/InfrastructureDeploymentTestCaseTest.java
index 5928c15..2059d76 100644
--- a/test-framework/src/test/java/org/apache/brooklyn/test/framework/InfrastructureDeploymentTestCaseTest.java
+++ b/test-framework/src/test/java/org/apache/brooklyn/test/framework/InfrastructureDeploymentTestCaseTest.java
@@ -182,7 +182,7 @@ public class InfrastructureDeploymentTestCaseTest {
app.start(ImmutableList.of(app.newSimulatedLocation()));
Asserts.shouldHaveFailedPreviously();
} catch (Throwable throwable) {
- Asserts.expectedFailure(throwable);
+ Asserts.expectedFailureContains(throwable, "EntitySpec", "not configured");
}
assertThat(infrastructureDeploymentTestCase.sensors().get(SERVICE_UP)).isFalse();
@@ -201,7 +201,7 @@ public class InfrastructureDeploymentTestCaseTest {
app.start(ImmutableList.of(app.newSimulatedLocation()));
Asserts.shouldHaveFailedPreviously();
} catch (Throwable throwable) {
- Asserts.expectedFailure(throwable);
+ Asserts.expectedFailureContains(throwable, "entity.specs", "List", "not configured");
}
assertThat(infrastructureDeploymentTestCase.sensors().get(SERVICE_UP)).isFalse();
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e1e3764e/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java b/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java
index 31ccfaa..7653370 100644
--- a/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java
+++ b/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java
@@ -47,14 +47,16 @@ import com.google.common.collect.Lists;
public class Exceptions {
- private static final List<Class<? extends Throwable>> ALWAYS_BORING_THROWABLE_SUPERTYPES = ImmutableList.<Class<? extends Throwable>>of(
+ /** {@link Throwable} types whose existence is unhelpful in a <b>message</b>. */
+ private static final List<Class<? extends Throwable>> ALWAYS_BORING_MESSAGE_THROWABLE_SUPERTYPES = ImmutableList.<Class<? extends Throwable>>of(
ExecutionException.class, InvocationTargetException.class, UndeclaredThrowableException.class);
-
+ /** As {@link #ALWAYS_BORING_MESSAGE_THROWABLE_SUPERTYPES} but might carry an interesting message. */
private static final List<Class<? extends Throwable>> BORING_IF_NO_MESSAGE_THROWABLE_SUPERTYPES = ImmutableList.<Class<? extends Throwable>>of(
PropagatedRuntimeException.class);
- private static boolean isBoring(Throwable t) {
- for (Class<? extends Throwable> type: ALWAYS_BORING_THROWABLE_SUPERTYPES)
+ /** NB: might be useful for stack trace, e.g. {@link ExecutionException} */
+ private static boolean isBoringForMessage(Throwable t) {
+ for (Class<? extends Throwable> type: ALWAYS_BORING_MESSAGE_THROWABLE_SUPERTYPES)
if (type.isInstance(t)) return true;
if (Strings.isBlank(t.getMessage())) {
for (Class<? extends Throwable> type: BORING_IF_NO_MESSAGE_THROWABLE_SUPERTYPES)
@@ -63,10 +65,10 @@ public class Exceptions {
return false;
}
- private static final Predicate<Throwable> IS_THROWABLE_BORING = new Predicate<Throwable>() {
+ private static final Predicate<Throwable> IS_THROWABLE_BORING_FOR_MESSAGE = new Predicate<Throwable>() {
@Override
public boolean apply(Throwable input) {
- return isBoring(input);
+ return isBoringForMessage(input);
}
};
@@ -77,11 +79,13 @@ public class Exceptions {
private static List<Class<? extends Throwable>> BORING_PREFIX_THROWABLE_SUPERTYPES = ImmutableList.<Class<? extends Throwable>>of(
ClassCastException.class, CompoundRuntimeException.class, PropagatedRuntimeException.class);
- /** Returns whether this is throwable either known to be boring or to have an unhelpful type name (prefix)
- * which should be suppressed. null is accepted but treated as not boring. */
+ /** Returns whether the prefix is throwable either known to be boring or to have an unhelpful type name (prefix)
+ * which should be suppressed in <b>messages</b>. (They may be important in stack traces.)
+ * <p>
+ * null is accepted but treated as not boring. */
public static boolean isPrefixBoring(Throwable t) {
if (t==null) return false;
- if (isBoring(t))
+ if (isBoringForMessage(t))
return true;
if (t instanceof UserFacingException) return true;
for (Class<? extends Throwable> type: BORING_PREFIX_THROWABLE_EXACT_TYPES)
@@ -186,12 +190,23 @@ public class Exceptions {
return Iterables.tryFind(getCausalChain(from), filter).orNull();
}
- /** returns the first exception in the call chain which is not of common uninteresting types
- * (ie excluding ExecutionException and PropagatedRuntimeExceptions);
- * or the original throwable if all are uninteresting
+ /** returns the first exception in the call chain which whose message is potentially interesting,
+ * in the sense that it is has some chance of giving helpful information as the cause.
+ * <p>
+ * more specifically this drops those which typically wrap such causes giving chain / thread info,
+ * reporting rather than causal explanation or important context --
+ * ie excluding {@link ExecutionException} always,
+ * and {@link PropagatedRuntimeException} if it has no message,
+ * and similar such.
+ * <p>
+ * if all are "uninteresting" in this sense (which should not normally be the case)
+ * this method just returns the original.
+ * <p>
+ * often looking for a {@link UserFacingException} eg using {@link #getFirstThrowableOfType(Throwable, Class)}
+ * is a better way to give a user-facing message.
*/
public static Throwable getFirstInteresting(Throwable throwable) {
- return Iterables.tryFind(getCausalChain(throwable), Predicates.not(IS_THROWABLE_BORING)).or(throwable);
+ return Iterables.tryFind(getCausalChain(throwable), Predicates.not(IS_THROWABLE_BORING_FOR_MESSAGE)).or(throwable);
}
/** creates (but does not throw) a new {@link PropagatedRuntimeException} whose
@@ -220,7 +235,7 @@ public class Exceptions {
int collapseCount = 0;
boolean messageIsFinal = false;
// remove boring stack traces at the head
- while (isBoring(collapsed) && !messageIsFinal) {
+ while (isBoringForMessage(collapsed) && !messageIsFinal) {
collapseCount++;
Throwable cause = collapsed.getCause();
if (cause==null) {
http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/e1e3764e/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/RuntimeInterruptedException.java
----------------------------------------------------------------------
diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/RuntimeInterruptedException.java b/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/RuntimeInterruptedException.java
index 8a4482a..c0cde50 100644
--- a/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/RuntimeInterruptedException.java
+++ b/utils/common/src/main/java/org/apache/brooklyn/util/exceptions/RuntimeInterruptedException.java
@@ -33,12 +33,17 @@ public class RuntimeInterruptedException extends RuntimeException {
private static final long serialVersionUID = 915050245927866175L;
- public RuntimeInterruptedException(Exception cause) {
+ public RuntimeInterruptedException(InterruptedException cause) {
super(cause);
Thread.currentThread().interrupt();
}
- public RuntimeInterruptedException(String msg, Exception cause) {
+ public RuntimeInterruptedException(String msg, InterruptedException cause) {
+ super(msg, cause);
+ Thread.currentThread().interrupt();
+ }
+
+ public RuntimeInterruptedException(String msg, RuntimeInterruptedException cause) {
super(msg, cause);
Thread.currentThread().interrupt();
}