From 97542f208267cb5392fc8c8d9baf6c1d3ee4ae32 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Thu, 13 Oct 2016 11:58:23 -0400 Subject: [PATCH] Fix FindBugs warnings in sal-akk-raft Fixed FindBugs warnings in sal-akk-raft and enabled the maven plugin to run and fail the build on violations. Some warnngs were suppressed with justification provided. Change-Id: I96b4bb58e6f5a6c3d3d14dadb9567df58ef5905f Signed-off-by: Tom Pantelis --- opendaylight/md-sal/sal-akka-raft/pom.xml | 8 +++++++- .../cluster/raft/RaftActorRecoverySupport.java | 3 ++- .../controller/cluster/raft/Snapshot.java | 4 ++++ .../raft/base/messages/CaptureSnapshotReply.java | 7 +++++++ .../raft/client/messages/GetSnapshotReply.java | 4 ++++ .../cluster/raft/messages/AppendEntries.java | 2 +- .../cluster/raft/messages/InstallSnapshot.java | 7 +++++++ .../raft/persisted/ServerConfigurationPayload.java | 13 +++++++++---- .../cluster/raft/behaviors/SnapshotTrackerTest.java | 2 +- .../cluster/raft/utils/MessageCollectorActor.java | 7 ++----- 10 files changed, 44 insertions(+), 13 deletions(-) diff --git a/opendaylight/md-sal/sal-akka-raft/pom.xml b/opendaylight/md-sal/sal-akka-raft/pom.xml index c4943cd272..6f106159fa 100644 --- a/opendaylight/md-sal/sal-akka-raft/pom.xml +++ b/opendaylight/md-sal/sal-akka-raft/pom.xml @@ -157,7 +157,13 @@ checkstyle.violationSeverity=error - + + org.codehaus.mojo + findbugs-maven-plugin + + true + + diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorRecoverySupport.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorRecoverySupport.java index f0a7066d85..5e4e6571a1 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorRecoverySupport.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/RaftActorRecoverySupport.java @@ -11,6 +11,7 @@ import akka.persistence.RecoveryCompleted; import akka.persistence.SnapshotOffer; import com.google.common.base.Stopwatch; import java.io.ByteArrayInputStream; +import java.io.IOException; import java.io.ObjectInputStream; import java.util.Collections; import org.opendaylight.controller.cluster.PersistentDataProvider; @@ -96,7 +97,7 @@ class RaftActorRecoverySupport { log.debug("{}: Deserialized restore snapshot: {}", context.getId(), snapshot); context.getSnapshotManager().apply(new ApplySnapshot(snapshot)); - } catch (Exception e) { + } catch (RuntimeException | ClassNotFoundException | IOException e) { log.error("{}: Error deserializing snapshot restore", context.getId(), e); } } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/Snapshot.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/Snapshot.java index 24132ea999..2677baff6d 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/Snapshot.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/Snapshot.java @@ -7,6 +7,7 @@ */ package org.opendaylight.controller.cluster.raft; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.Serializable; import java.util.List; import org.opendaylight.controller.cluster.raft.persisted.ServerConfigurationPayload; @@ -62,6 +63,9 @@ public class Snapshot implements Serializable { electionTerm, electionVotedFor, serverConfig); } + @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Exposes a mutable object stored in a field but " + + "this is OK since this class is merely a DTO and does not process the byte[] internally. " + + "Also it would be inefficient to create a return copy as the byte[] could be large.") public byte[] getState() { return state; } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/base/messages/CaptureSnapshotReply.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/base/messages/CaptureSnapshotReply.java index 82f3e0dce0..1e573014a9 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/base/messages/CaptureSnapshotReply.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/base/messages/CaptureSnapshotReply.java @@ -7,14 +7,21 @@ */ package org.opendaylight.controller.cluster.raft.base.messages; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; public class CaptureSnapshotReply { private final byte [] snapshot; + @SuppressFBWarnings(value = "EI_EXPOSE_REP2", justification = "Stores a reference to an externally mutable byte[] " + + "object but this is OK since this class is merely a DTO and does not process byte[] internally. " + + "Also it would be inefficient to create a copy as the byte[] could be large.") public CaptureSnapshotReply(byte [] snapshot) { this.snapshot = snapshot; } + @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Exposes a mutable object stored in a field but " + + "this is OK since this class is merely a DTO and does not process the byte[] internally. " + + "Also it would be inefficient to create a return copy as the byte[] could be large.") public byte [] getSnapshot() { return snapshot; } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/client/messages/GetSnapshotReply.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/client/messages/GetSnapshotReply.java index f3521deb3b..2918c5e752 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/client/messages/GetSnapshotReply.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/client/messages/GetSnapshotReply.java @@ -8,6 +8,7 @@ package org.opendaylight.controller.cluster.raft.client.messages; import com.google.common.base.Preconditions; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import javax.annotation.Nonnull; /** @@ -29,6 +30,9 @@ public class GetSnapshotReply { return id; } + @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Exposes a mutable object stored in a field but " + + "this is OK since this class is merely a DTO and does not process the byte[] internally. " + + "Also it would be inefficient to create a return copy as the byte[] could be large.") @Nonnull public byte[] getSnapshot() { return snapshot; diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/AppendEntries.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/AppendEntries.java index 487ea9b09d..7ebc857444 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/AppendEntries.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/AppendEntries.java @@ -35,7 +35,7 @@ public class AppendEntries extends AbstractRaftRPC { private final long prevLogTerm; // log entries to store (empty for heart beat - may send more than one for efficiency) - private transient List entries; + private final List entries; // leader's commitIndex private final long leaderCommit; diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/InstallSnapshot.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/InstallSnapshot.java index 4528dcbf5e..b64e902f3f 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/InstallSnapshot.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/InstallSnapshot.java @@ -9,6 +9,7 @@ package org.opendaylight.controller.cluster.raft.messages; import com.google.common.base.Optional; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.Externalizable; import java.io.IOException; import java.io.ObjectInput; @@ -30,6 +31,9 @@ public class InstallSnapshot extends AbstractRaftRPC { private final Optional lastChunkHashCode; private final Optional serverConfig; + @SuppressFBWarnings(value = "EI_EXPOSE_REP2", justification = "Stores a reference to an externally mutable byte[] " + + "object but this is OK since this class is merely a DTO and does not process byte[] internally. " + + "Also it would be inefficient to create a copy as the byte[] could be large.") public InstallSnapshot(long term, String leaderId, long lastIncludedIndex, long lastIncludedTerm, byte[] data, int chunkIndex, int totalChunks, Optional lastChunkHashCode, Optional serverConfig) { @@ -62,6 +66,9 @@ public class InstallSnapshot extends AbstractRaftRPC { return lastIncludedTerm; } + @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Exposes a mutable object stored in a field but " + + "this is OK since this class is merely a DTO and does not process the byte[] internally. " + + "Also it would be inefficient to create a return copy as the byte[] could be large.") public byte[] getData() { return data; } diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/persisted/ServerConfigurationPayload.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/persisted/ServerConfigurationPayload.java index 0f206b5d39..7138b634f4 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/persisted/ServerConfigurationPayload.java +++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/persisted/ServerConfigurationPayload.java @@ -8,6 +8,7 @@ package org.opendaylight.controller.cluster.raft.persisted; import com.google.common.collect.ImmutableList; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.ByteArrayOutputStream; import java.io.Externalizable; import java.io.IOException; @@ -72,21 +73,24 @@ public final class ServerConfigurationPayload extends Payload implements Persist private static final Logger LOG = LoggerFactory.getLogger(ServerConfigurationPayload.class); private static final long serialVersionUID = 1L; + @SuppressFBWarnings(value = "SE_BAD_FIELD", justification = "This field is not Serializable but this class " + + "implements writeReplace to delegate serialization to a Proxy class and thus instances of this class " + + "aren't serialized. FindBugs does not recognize this.") private final List serverConfig; private final boolean migrated; private int serializedSize = -1; - private ServerConfigurationPayload(final @Nonnull List serverConfig, boolean migrated) { + private ServerConfigurationPayload(@Nonnull final List serverConfig, boolean migrated) { this.serverConfig = ImmutableList.copyOf(serverConfig); this.migrated = migrated; } - public ServerConfigurationPayload(final @Nonnull List serverConfig) { + public ServerConfigurationPayload(@Nonnull final List serverConfig) { this(serverConfig, false); } @Deprecated - public static ServerConfigurationPayload createMigrated(final @Nonnull List serverConfig) { + public static ServerConfigurationPayload createMigrated(@Nonnull final List serverConfig) { return new ServerConfigurationPayload(serverConfig, true); } @@ -96,7 +100,8 @@ public final class ServerConfigurationPayload extends Payload implements Persist return migrated; } - public @Nonnull List getServerConfig() { + @Nonnull + public List getServerConfig() { return serverConfig; } diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/SnapshotTrackerTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/SnapshotTrackerTest.java index 99c7b58724..7591bb51d8 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/SnapshotTrackerTest.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/behaviors/SnapshotTrackerTest.java @@ -65,7 +65,7 @@ public class SnapshotTrackerTest { tracker2.addChunk(3, chunk3, Optional.absent()); Assert.fail(); } catch (SnapshotTracker.InvalidChunkException e) { - e.getMessage().startsWith("Invalid chunk"); + // expected } // The first chunk's index must at least be FIRST_CHUNK_INDEX diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/MessageCollectorActor.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/MessageCollectorActor.java index e947aede3b..68d6b619cd 100644 --- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/MessageCollectorActor.java +++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/utils/MessageCollectorActor.java @@ -38,12 +38,9 @@ public class MessageCollectorActor extends UntypedActor { private final List messages = new ArrayList<>(); @Override public void onReceive(Object message) throws Exception { - if (message.equals(ARE_YOU_READY)) { + if (ARE_YOU_READY.equals(message)) { getSender().tell("yes", getSelf()); - return; - } - - if (GET_ALL_MESSAGES.equals(message)) { + } else if (GET_ALL_MESSAGES.equals(message)) { getSender().tell(new ArrayList<>(messages), getSelf()); } else if (CLEAR_MESSAGES.equals(message)) { clear(); -- 2.36.6