From 7b66583e80a8850bcf91c325ee6f6dcfa5036f5f Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sat, 13 Apr 2019 14:55:13 +0200 Subject: [PATCH] Do not allow persistence callbacks to throw Exception Since we are indirecting through an executor, we are forced to wrap any exception -- just do not bother, as the callbacks are executed in the context of an actor anyway (dealing with RuntimeExceptions) and users are not throwing checked exceptions. Change-Id: I6cea19ab7192fa42ad3c346d554411cd0d558a64 Signed-off-by: Robert Varga --- .../controller/cluster/raft/ReplicatedLog.java | 6 +++--- .../cluster/raft/ReplicatedLogImpl.java | 16 +++++----------- .../cluster/raft/behaviors/Follower.java | 4 ++-- .../raft/AbstractReplicatedLogImplTest.java | 7 +++++-- .../cluster/raft/MockRaftActorContext.java | 11 ++--------- .../cluster/raft/ReplicatedLogImplTest.java | 7 ++++--- 6 files changed, 21 insertions(+), 30 deletions(-) diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLog.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLog.java index a05dbc662f..1a0b38f778 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLog.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLog.java @@ -7,8 +7,8 @@ */ package org.opendaylight.controller.cluster.raft; -import akka.japi.Procedure; import java.util.List; +import java.util.function.Consumer; import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; @@ -88,7 +88,7 @@ public interface ReplicatedLog { * Appends an entry to the in-memory log and persists it as well. * * @param replicatedLogEntry the entry to append - * @param callback the Procedure to be notified when persistence is complete (optional). + * @param callback the callback to be notified when persistence is complete (optional). * @param doAsync if true, the persistent actor can receive subsequent messages to process in between the persist * call and the execution of the associated callback. If false, subsequent messages are stashed and get * delivered after persistence is complete and the associated callback is executed. In either case the @@ -96,7 +96,7 @@ public interface ReplicatedLog { * @return true if the entry was successfully appended, false otherwise. */ boolean appendAndPersist(@NonNull ReplicatedLogEntry replicatedLogEntry, - @Nullable Procedure callback, boolean doAsync); + @Nullable Consumer callback, boolean doAsync); /** * Returns a list of log entries starting from the given index to the end of the log. diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImpl.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImpl.java index c53cad68c5..c22f6f431b 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImpl.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImpl.java @@ -9,9 +9,9 @@ package org.opendaylight.controller.cluster.raft; import static java.util.Objects.requireNonNull; -import akka.japi.Procedure; import java.util.Collections; import java.util.List; +import java.util.function.Consumer; import org.opendaylight.controller.cluster.raft.persisted.DeleteEntries; import org.opendaylight.controller.cluster.raft.persisted.Snapshot; @@ -93,7 +93,7 @@ final class ReplicatedLogImpl extends AbstractReplicatedLogImpl { @Override public boolean appendAndPersist(final ReplicatedLogEntry replicatedLogEntry, - final Procedure callback, final boolean doAsync) { + final Consumer callback, final boolean doAsync) { context.getLogger().debug("{}: Append log entry and persist {} ", context.getId(), replicatedLogEntry); @@ -112,24 +112,18 @@ final class ReplicatedLogImpl extends AbstractReplicatedLogImpl { } private void persistCallback(final ReplicatedLogEntry persistedLogEntry, - final Procedure callback) { + final Consumer callback) { context.getExecutor().execute(() -> syncPersistCallback(persistedLogEntry, callback)); } - @SuppressWarnings("checkstyle:illegalCatch") private void syncPersistCallback(final ReplicatedLogEntry persistedLogEntry, - final Procedure callback) { + final Consumer callback) { context.getLogger().debug("{}: persist complete {}", context.getId(), persistedLogEntry); dataSizeSinceLastSnapshot += persistedLogEntry.size(); if (callback != null) { - try { - callback.apply(persistedLogEntry); - } catch (Exception e) { - context.getLogger().error("{}: persist callback failed", context.getId(), e); - throw new IllegalStateException("Persist callback failed", e); - } + callback.accept(persistedLogEntry); } } } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java index 4fa4dbfb57..b193a549f7 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/Follower.java @@ -14,7 +14,6 @@ import akka.cluster.Cluster; import akka.cluster.ClusterEvent.CurrentClusterState; import akka.cluster.Member; import akka.cluster.MemberStatus; -import akka.japi.Procedure; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Stopwatch; import java.io.IOException; @@ -24,6 +23,7 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; import org.eclipse.jdt.annotation.Nullable; import org.opendaylight.controller.cluster.messaging.MessageAssembler; import org.opendaylight.controller.cluster.raft.RaftActorContext; @@ -303,7 +303,7 @@ public class Follower extends AbstractRaftActorBehavior { // applied to the state already, as the persistence callback occurs async, and we want those entries // purged from the persisted log as well. final AtomicBoolean shouldCaptureSnapshot = new AtomicBoolean(false); - final Procedure appendAndPersistCallback = logEntry -> { + final Consumer appendAndPersistCallback = logEntry -> { final List entries = appendEntries.getEntries(); final ReplicatedLogEntry lastEntryToAppend = entries.get(entries.size() - 1); if (shouldCaptureSnapshot.get() && logEntry == lastEntryToAppend) { diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/AbstractReplicatedLogImplTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/AbstractReplicatedLogImplTest.java index 8ff6831024..5b72cb3117 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/AbstractReplicatedLogImplTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/AbstractReplicatedLogImplTest.java @@ -13,10 +13,10 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import akka.japi.Procedure; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Consumer; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -325,8 +325,11 @@ public class AbstractReplicatedLogImplTest { } @Override - public boolean appendAndPersist(ReplicatedLogEntry replicatedLogEntry, Procedure callback, + public boolean appendAndPersist(ReplicatedLogEntry replicatedLogEntry, Consumer callback, boolean doAsync) { + if (callback != null) { + callback.accept(replicatedLogEntry); + } return true; } diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/MockRaftActorContext.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/MockRaftActorContext.java index 1935e26503..8c17e1e8e8 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/MockRaftActorContext.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/MockRaftActorContext.java @@ -12,7 +12,6 @@ import akka.actor.ActorRef; import akka.actor.ActorSelection; import akka.actor.ActorSystem; import akka.actor.Props; -import akka.japi.Procedure; import com.google.common.io.ByteSource; import com.google.common.util.concurrent.MoreExecutors; import java.io.IOException; @@ -190,17 +189,11 @@ public class MockRaftActorContext extends RaftActorContextImpl { @Override @SuppressWarnings("checkstyle:IllegalCatch") public boolean appendAndPersist(final ReplicatedLogEntry replicatedLogEntry, - final Procedure callback, final boolean doAsync) { + final Consumer callback, final boolean doAsync) { append(replicatedLogEntry); if (callback != null) { - try { - callback.apply(replicatedLogEntry); - } catch (RuntimeException e) { - throw e; - } catch (Exception e) { - throw new RuntimeException(e); - } + callback.accept(replicatedLogEntry); } return true; diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImplTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImplTest.java index 276ace15b5..8e9c6c9271 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImplTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/ReplicatedLogImplTest.java @@ -19,6 +19,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import akka.japi.Procedure; import com.google.common.util.concurrent.MoreExecutors; import java.util.Collections; +import java.util.function.Consumer; import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; @@ -92,12 +93,12 @@ public class ReplicatedLogImplTest { reset(mockPersistence); ReplicatedLogEntry logEntry2 = new SimpleReplicatedLogEntry(2, 1, new MockPayload("2")); - Procedure mockCallback = mock(Procedure.class); + Consumer mockCallback = mock(Consumer.class); log.appendAndPersist(logEntry2, mockCallback, true); verifyPersist(logEntry2); - verify(mockCallback).apply(same(logEntry2)); + verify(mockCallback).accept(same(logEntry2)); assertEquals("size", 2, log.size()); } @@ -107,7 +108,7 @@ public class ReplicatedLogImplTest { public void testAppendAndPersisWithDuplicateEntry() throws Exception { ReplicatedLog log = ReplicatedLogImpl.newInstance(context); - Procedure mockCallback = mock(Procedure.class); + Consumer mockCallback = mock(Consumer.class); ReplicatedLogEntry logEntry = new SimpleReplicatedLogEntry(1, 1, new MockPayload("1")); log.appendAndPersist(logEntry, mockCallback, true); -- 2.36.6