Counter steer against too noisy logs

src/main/java/org/swisspush/reststorage/RestStorageMod.java

@@ -5,20 +5,31 @@
 import io.vertx.core.http.HttpServerRequest;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.swisspush.reststorage.exception.RestStorageExceptionFactory;
 import org.swisspush.reststorage.redis.RedisProvider;
 import org.swisspush.reststorage.redis.RedisStorage;
 import org.swisspush.reststorage.util.ModuleConfiguration;
 
+import static org.swisspush.reststorage.exception.RestStorageExceptionFactory.newRestStorageThriftyExceptionFactory;
+
 public class RestStorageMod extends AbstractVerticle {
 
     private final Logger log = LoggerFactory.getLogger(RestStorageMod.class);
 
     private RedisProvider redisProvider;
+    private final RestStorageExceptionFactory exceptionFactory;
 
-    public RestStorageMod() {}
+    public RestStorageMod() {
+        this.exceptionFactory = newRestStorageThriftyExceptionFactory();
+    }
 
-    public RestStorageMod(RedisProvider redisProvider) {
+    public RestStorageMod(
+        RedisProvider redisProvider,
+        RestStorageExceptionFactory exceptionFactory
+    ) {
+        assert exceptionFactory != null;
         this.redisProvider = redisProvider;
+        this.exceptionFactory = exceptionFactory;
     }
 
     @Override
@@ -84,7 +95,7 @@ private Future<RedisStorage> createRedisStorage(Vertx vertx, ModuleConfiguration
 
         redisProvider.redis().onComplete(event -> {
             if(event.succeeded()) {
-                initPromise.complete(new RedisStorage(vertx, moduleConfiguration, redisProvider));
+                initPromise.complete(new RedisStorage(vertx, moduleConfiguration, redisProvider, exceptionFactory));
             } else {
                 initPromise.fail(new Exception(event.cause()));
             }

src/main/java/org/swisspush/reststorage/exception/RestStorageExceptionFactory.java

@@ -0,0 +1,39 @@
+package org.swisspush.reststorage.exception;
+
+import io.vertx.core.eventbus.ReplyException;
+import io.vertx.core.eventbus.ReplyFailure;
+
+/**
+ * Applies dependency inversion for exception instantiation.
+ *
+ * This class did arise because we had different use cases in different
+ * applications. One of them has the need to perform fine-grained error
+ * reporting. Whereas in the other application this led to performance issues.
+ * So now through this abstraction, both applications can choose the behavior
+ * they need.
+ *
+ * If dependency-injection gets applied properly, an app can even provide its
+ * custom implementation to fine-tune the exact behavior even further.
+ */
+public interface RestStorageExceptionFactory {
+
+    public Exception newException(String message, Throwable cause);
+
+    public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message);
+
+
+    /**
+     * See {@link RestStorageThriftyExceptionFactory}.
+     */
+    public static RestStorageExceptionFactory newRestStorageThriftyExceptionFactory() {
+        return new RestStorageThriftyExceptionFactory();
+    }
+
+    /**
+     * See {@link RestStorageWastefulExceptionFactory}.
+     */
+    public static RestStorageExceptionFactory newRestStorageWastefulExceptionFactory() {
+        return new RestStorageWastefulExceptionFactory();
+    }
+
+}

src/main/java/org/swisspush/reststorage/exception/RestStorageNoStackReplyException.java

@@ -0,0 +1,30 @@
+package org.swisspush.reststorage.exception;
+
+import io.vertx.core.eventbus.ReplyFailure;
+
+/**
+ * There was once a fix in vertx for this (https://github.com/eclipse-vertx/vert.x/issues/4840)
+ * but for whatever reason in our case we still see stack-trace recordings. Passing
+ * this subclass to {@link io.vertx.core.eventbus.Message#reply(Object)} seems to
+ * do the trick.
+ */
+public class RestStorageNoStackReplyException extends io.vertx.core.eventbus.ReplyException {
+
+    public RestStorageNoStackReplyException(ReplyFailure failureType, int failureCode, String message) {
+        super(failureType, failureCode, message);
+    }
+
+    public RestStorageNoStackReplyException(ReplyFailure failureType, String message) {
+        this(failureType, -1, message);
+    }
+
+    public RestStorageNoStackReplyException(ReplyFailure failureType) {
+        this(failureType, -1, null);
+    }
+
+    @Override
+    public Throwable fillInStackTrace() {
+        return this;
+    }
+
+}

src/main/java/org/swisspush/reststorage/exception/RestStorageNoStacktraceException.java

@@ -0,0 +1,28 @@
+package org.swisspush.reststorage.exception;
+
+/**
+ * Basically same as in vertx, But adding the forgotten contructors.
+ */
+public class RestStorageNoStacktraceException extends RuntimeException {
+
+    public RestStorageNoStacktraceException() {
+    }
+
+    public RestStorageNoStacktraceException(String message) {
+        super(message);
+    }
+
+    public RestStorageNoStacktraceException(String message, Throwable cause) {
+        super(message, cause);
+    }
+
+    public RestStorageNoStacktraceException(Throwable cause) {
+        super(cause);
+    }
+
+    @Override
+    public Throwable fillInStackTrace() {
+        return this;
+    }
+
+}

src/main/java/org/swisspush/reststorage/exception/RestStorageThriftyExceptionFactory.java

@@ -0,0 +1,30 @@
+package org.swisspush.reststorage.exception;
+
+import io.vertx.core.eventbus.ReplyException;
+import io.vertx.core.eventbus.ReplyFailure;
+
+/**
+ * Trades maintainability for speed. For example prefers lightweight
+ * exceptions without stacktrace recording. It may even decide to drop 'cause'
+ * and 'suppressed' exceptions. Or to make other optimizations towards
+ * performance. If an app needs more error details it should use
+ * {@link RestStorageWastefulExceptionFactory}. If none of those fits the apps
+ * needs, it can provide its own implementation.
+ */
+class RestStorageThriftyExceptionFactory implements RestStorageExceptionFactory {
+
+    RestStorageThriftyExceptionFactory() {
+    }
+
+    @Override
+    public Exception newException(String message, Throwable cause) {
+        if (cause instanceof Exception) return (Exception) cause;
+        return new RestStorageNoStacktraceException(message, cause);
+    }
+
+    @Override
+    public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message) {
+        return new RestStorageNoStackReplyException(failureType, failureCode, message);
+    }
+
+}

src/main/java/org/swisspush/reststorage/exception/RestStorageWastefulExceptionFactory.java

@@ -0,0 +1,30 @@
+package org.swisspush.reststorage.exception;
+
+import io.vertx.core.eventbus.ReplyException;
+import io.vertx.core.eventbus.ReplyFailure;
+
+/**
+ * Trades speed for maintainability. For example invests more resources like
+ * recording stack traces (which likely provocates more logs) to get easier
+ * to debug error messages and better hints of what is happening. It also
+ * keeps details like 'causes' and 'suppressed' exceptions. If an app needs
+ * more error details it should use {@link RestStorageThriftyExceptionFactory}
+ * instead. If none of those impls fit the apps needs, it can provide its own
+ * implementation.
+ */
+class RestStorageWastefulExceptionFactory implements RestStorageExceptionFactory {
+
+    RestStorageWastefulExceptionFactory() {
+    }
+
+    @Override
+    public Exception newException(String message, Throwable cause) {
+        return new Exception(message, cause);
+    }
+
+    @Override
+    public ReplyException newReplyException(ReplyFailure failureType, int failureCode, String message) {
+        return new ReplyException(failureType, failureCode, message);
+    }
+
+}

src/main/java/org/swisspush/reststorage/lock/impl/RedisBasedLock.java

@@ -9,6 +9,7 @@
 import io.vertx.redis.client.Response;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.swisspush.reststorage.exception.RestStorageExceptionFactory;
 import org.swisspush.reststorage.redis.RedisProvider;
 import org.swisspush.reststorage.lock.Lock;
 import org.swisspush.reststorage.lock.lua.LockLuaScripts;
@@ -34,9 +35,14 @@ public class RedisBasedLock implements Lock {
 
     private final LuaScriptState releaseLockLuaScriptState;
     private final RedisProvider redisProvider;
+    private final RestStorageExceptionFactory exceptionFactory;
 
-    public RedisBasedLock(RedisProvider redisProvider) {
+    public RedisBasedLock(
+        RedisProvider redisProvider,
+        RestStorageExceptionFactory exceptionFactory
+    ) {
         this.redisProvider = redisProvider;
+        this.exceptionFactory = exceptionFactory;
         this.releaseLockLuaScriptState = new LuaScriptState(LockLuaScripts.LOCK_RELEASE, redisProvider, false);
     }
 
@@ -74,7 +80,7 @@ public Future<Boolean> acquireLock(String lock, String token, long lockExpiryMs)
                     promise.complete(false);
                 }
             } else {
-                promise.fail(new Exception("stacktrace", event.cause()));
+                promise.fail(exceptionFactory.newException("redisSetWithOptions() failed", event.cause()));
             }
         });
         return promise.future();

src/main/java/org/swisspush/reststorage/redis/RedisStorage.java

@@ -23,6 +23,7 @@
 import org.swisspush.reststorage.DocumentResource;
 import org.swisspush.reststorage.Resource;
 import org.swisspush.reststorage.Storage;
+import org.swisspush.reststorage.exception.RestStorageExceptionFactory;
 import org.swisspush.reststorage.lock.Lock;
 import org.swisspush.reststorage.lock.impl.RedisBasedLock;
 import org.swisspush.reststorage.util.GZIPUtil;
@@ -39,17 +40,14 @@
 import java.text.DecimalFormat;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.UUID;
-import java.util.stream.Stream;
 
 import static org.swisspush.reststorage.redis.RedisUtils.toPayload;
 
@@ -89,7 +87,12 @@ public class RedisStorage implements Storage {
     private final String ID;
     private final String hostAndPort;
 
-    public RedisStorage(Vertx vertx, ModuleConfiguration config, RedisProvider redisProvider) {
+    public RedisStorage(
+        Vertx vertx,
+        ModuleConfiguration config,
+        RedisProvider redisProvider,
+        RestStorageExceptionFactory exceptionFactory
+    ) {
         this.expirableSet = config.getExpirablePrefix();
         this.redisResourcesPrefix = config.getResourcesPrefix();
         this.redisCollectionsPrefix = config.getCollectionsPrefix();
@@ -106,7 +109,7 @@ public RedisStorage(Vertx vertx, ModuleConfiguration config, RedisProvider redis
 
         this.ID = UUID.randomUUID().toString();
         this.hostAndPort = config.getRedisHost() + ":" + config.getPort();
-        this.lock = new RedisBasedLock(redisProvider);
+        this.lock = new RedisBasedLock(redisProvider, exceptionFactory);
 
         // load all the lua scripts
         LuaScriptState luaGetScriptState = new LuaScriptState(LuaScript.GET, false);

src/test/java/org/swisspush/reststorage/lock/impl/RedisBasedLockTest.java

@@ -15,13 +15,15 @@
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.swisspush.reststorage.exception.RestStorageExceptionFactory;
 import redis.clients.jedis.Jedis;
 import redis.clients.jedis.exceptions.JedisConnectionException;
 
 import java.time.Duration;
 
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static org.awaitility.Awaitility.await;
+import static org.swisspush.reststorage.exception.RestStorageExceptionFactory.newRestStorageWastefulExceptionFactory;
 
 /**
  * Tests for the {@link RedisBasedLock} class
@@ -45,7 +47,7 @@ public class RedisBasedLockTest {
     public static void setupLock(){
         vertx = Vertx.vertx();
         RedisAPI redisAPI = RedisAPI.api(new RedisClient(vertx, new NetClientOptions(), new PoolOptions(), new RedisStandaloneConnectOptions(), TracingPolicy.IGNORE));
-        redisBasedLock = new RedisBasedLock(() -> Future.succeededFuture(redisAPI));
+        redisBasedLock = new RedisBasedLock(() -> Future.succeededFuture(redisAPI), newRestStorageWastefulExceptionFactory());
     }
 
     @Before

src/test/java/org/swisspush/reststorage/redis/RedisStorageTest.java

@@ -27,6 +27,7 @@
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
+import static org.swisspush.reststorage.exception.RestStorageExceptionFactory.newRestStorageWastefulExceptionFactory;
 
 /**
  * Tests for the {@link RedisStorage} class
@@ -45,8 +46,9 @@ public void setUp(TestContext context) {
         redisAPI = Mockito.mock(RedisAPI.class);
         redisProvider = Mockito.mock(RedisProvider.class);
         when(redisProvider.redis()).thenReturn(Future.succeededFuture(redisAPI));
+        var exceptionFactory = newRestStorageWastefulExceptionFactory();
 
-        storage = new RedisStorage(mock(Vertx.class), new ModuleConfiguration(), redisProvider);
+        storage = new RedisStorage(mock(Vertx.class), new ModuleConfiguration(), redisProvider, exceptionFactory);
     }