From 4258ff0d490fc27658ab53dd71bf96c7aa981b13 Mon Sep 17 00:00:00 2001 From: Tom Pantelis Date: Thu, 27 Oct 2016 13:06:33 -0400 Subject: [PATCH] Fix FindBugs warnings in sal-remoterpc-connector and enable enforcement Warnings fixed: - RemoteRpcImplementation: use of 'error' known to be null - RpcBroker, RpcRegistry: The Creator class has non-Serializable field. Removed the Creator class and used Props that creates by reflection. - RpcBroker: use of 'result' that is marked as @Nullable - RpcBroker: redundant check of 'result.getErrors()' that is known to be non-null (marked as @Nonnull). - Gossiper, RemoteRpcRegistryMXBeanImpl: use entrySet iterator instead of keySet and get. - Messages: redundant specification of implements Serializable - LatestEntryRoutingLogic: Comparator should also implement Serializable in case TreeSet is serialized. This isn't the case here but it doesn't hurt to implement Serializable in lieu of supressing the warning. - LatestEntryRoutingLogic: Fixed potential null pointer de-reference in 'compare'. Change-Id: I8930c8975e1dd9179d78e74087b3994a365b90f8 Signed-off-by: Tom Pantelis --- .../md-sal/sal-remoterpc-connector/pom.xml | 7 ++++ .../remote/rpc/RemoteRpcImplementation.java | 2 +- .../remote/rpc/RemoteRpcProviderConfig.java | 5 +++ .../controller/remote/rpc/RpcBroker.java | 41 ++++++------------- .../remote/rpc/messages/ExecuteRpc.java | 4 ++ .../remote/rpc/messages/RpcResponse.java | 4 ++ .../remote/rpc/registry/RpcRegistry.java | 20 +-------- .../remote/rpc/registry/gossip/Gossiper.java | 13 +++--- .../remote/rpc/registry/gossip/Messages.java | 15 +++---- .../mbeans/RemoteRpcRegistryMXBeanImpl.java | 14 +++---- .../rpc/utils/LatestEntryRoutingLogic.java | 15 +++++-- 11 files changed, 66 insertions(+), 74 deletions(-) diff --git a/opendaylight/md-sal/sal-remoterpc-connector/pom.xml b/opendaylight/md-sal/sal-remoterpc-connector/pom.xml index 772d4461f0..c8251efdf5 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/pom.xml +++ b/opendaylight/md-sal/sal-remoterpc-connector/pom.xml @@ -204,6 +204,13 @@ checkstyle.violationSeverity=error + + org.codehaus.mojo + findbugs-maven-plugin + + true + + diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RemoteRpcImplementation.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RemoteRpcImplementation.java index e02c202551..11f145ce1e 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RemoteRpcImplementation.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RemoteRpcImplementation.java @@ -63,7 +63,7 @@ public class RemoteRpcImplementation implements DOMRpcImplementation { final List> routePairs = routes.getRouterWithUpdateTime(); if (routePairs == null || routePairs.isEmpty()) { frontEndFuture.failNow(new DOMRpcImplementationNotAvailableException( - "No local or remote implementation available for rpc %s", rpc.getType(), error)); + "No local or remote implementation available for rpc %s", rpc.getType())); } else { final ActorRef remoteImplRef = new LatestEntryRoutingLogic(routePairs).select(); final Object executeRpcMessage = ExecuteRpc.from(rpc, input); diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RemoteRpcProviderConfig.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RemoteRpcProviderConfig.java index 8cdf9c5ef1..b08df44559 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RemoteRpcProviderConfig.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RemoteRpcProviderConfig.java @@ -9,6 +9,7 @@ package org.opendaylight.controller.remote.rpc; import akka.util.Timeout; import com.typesafe.config.Config; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.concurrent.TimeUnit; import org.opendaylight.controller.cluster.common.actor.CommonConfig; import scala.concurrent.duration.FiniteDuration; @@ -82,6 +83,10 @@ public class RemoteRpcProviderConfig extends CommonConfig { /** * This is called via blueprint xml as the builder pattern can't be used. */ + @SuppressFBWarnings(value = "BC_UNCONFIRMED_CAST_OF_RETURN_VALUE", + justification = "Findbugs flags this as an unconfirmed cast of return value but the build method clearly " + + "returns RemoteRpcProviderConfig. Perhaps it's confused b/c the build method is overloaded and " + + "and differs in return type from the base class.") public static RemoteRpcProviderConfig newInstance(String actorSystemName, boolean metricCaptureEnabled, int mailboxCapacity) { return new Builder(actorSystemName).metricCaptureEnabled(metricCaptureEnabled) diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RpcBroker.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RpcBroker.java index 964a12bcf8..3d870c9941 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RpcBroker.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/RpcBroker.java @@ -10,23 +10,17 @@ package org.opendaylight.controller.remote.rpc; import akka.actor.ActorRef; import akka.actor.Props; -import akka.japi.Creator; import com.google.common.base.Preconditions; import com.google.common.base.Throwables; import com.google.common.util.concurrent.CheckedFuture; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; -import java.util.Arrays; -import java.util.Collection; import org.opendaylight.controller.cluster.common.actor.AbstractUntypedActor; import org.opendaylight.controller.md.sal.dom.api.DOMRpcException; import org.opendaylight.controller.md.sal.dom.api.DOMRpcResult; import org.opendaylight.controller.md.sal.dom.api.DOMRpcService; import org.opendaylight.controller.remote.rpc.messages.ExecuteRpc; import org.opendaylight.controller.remote.rpc.messages.RpcResponse; -import org.opendaylight.yangtools.yang.common.RpcError; -import org.opendaylight.yangtools.yang.common.RpcError.ErrorType; -import org.opendaylight.yangtools.yang.common.RpcResultBuilder; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; import org.opendaylight.yangtools.yang.model.api.SchemaPath; @@ -43,7 +37,7 @@ public class RpcBroker extends AbstractUntypedActor { public static Props props(final DOMRpcService rpcService) { Preconditions.checkNotNull(rpcService, "DOMRpcService can not be null"); - return Props.create(new RpcBrokerCreator(rpcService)); + return Props.create(RpcBroker.class, rpcService); } @Override @@ -67,14 +61,18 @@ public class RpcBroker extends AbstractUntypedActor { Futures.addCallback(future, new FutureCallback() { @Override public void onSuccess(final DOMRpcResult result) { - if (result.getErrors() != null && !result.getErrors().isEmpty()) { - final String message = String.format("Execution of RPC %s failed", msg.getRpc()); - Collection errors = result.getErrors(); - if (errors == null || errors.size() == 0) { - errors = Arrays.asList(RpcResultBuilder.newError(ErrorType.RPC, null, message)); - } + if (result == null) { + // This shouldn't happen but the FutureCallback annotates the result param with Nullable so + // handle null here to avoid FindBugs warning. + LOG.debug("Got null DOMRpcResult - sending null response for execute rpc : {}", msg.getRpc()); + sender.tell(new RpcResponse(null), self); + return; + } - sender.tell(new akka.actor.Status.Failure(new RpcErrorsException(message, errors)), self); + if (!result.getErrors().isEmpty()) { + final String message = String.format("Execution of RPC %s failed", msg.getRpc()); + sender.tell(new akka.actor.Status.Failure(new RpcErrorsException(message, + result.getErrors())), self); } else { LOG.debug("Sending response for execute rpc : {}", msg.getRpc()); @@ -97,19 +95,4 @@ public class RpcBroker extends AbstractUntypedActor { sender.tell(new akka.actor.Status.Failure(e), sender); } } - - private static class RpcBrokerCreator implements Creator { - private static final long serialVersionUID = 1L; - - final DOMRpcService rpcService; - - RpcBrokerCreator(final DOMRpcService rpcService) { - this.rpcService = rpcService; - } - - @Override - public RpcBroker create() throws Exception { - return new RpcBroker(rpcService); - } - } } diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/messages/ExecuteRpc.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/messages/ExecuteRpc.java index a5300a2722..4824cd71cc 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/messages/ExecuteRpc.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/messages/ExecuteRpc.java @@ -10,6 +10,7 @@ package org.opendaylight.controller.remote.rpc.messages; import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.Externalizable; import java.io.IOException; import java.io.ObjectInput; @@ -25,6 +26,9 @@ import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; public class ExecuteRpc implements Serializable { private static final long serialVersionUID = 1128904894827335676L; + @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 NormalizedNode inputNormalizedNode; private final QName rpc; diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/messages/RpcResponse.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/messages/RpcResponse.java index d46bf6ab32..2d86ac0f20 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/messages/RpcResponse.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/messages/RpcResponse.java @@ -7,6 +7,7 @@ */ package org.opendaylight.controller.remote.rpc.messages; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.Externalizable; import java.io.IOException; import java.io.ObjectInput; @@ -19,6 +20,9 @@ import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; public class RpcResponse implements Serializable { private static final long serialVersionUID = -4211279498688989245L; + @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 NormalizedNode resultNormalizedNode; public RpcResponse(@Nullable final NormalizedNode inputNormalizedNode) { diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/RpcRegistry.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/RpcRegistry.java index 43f5dbcdc0..fc5b618663 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/RpcRegistry.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/RpcRegistry.java @@ -10,7 +10,6 @@ package org.opendaylight.controller.remote.rpc.registry; import akka.actor.ActorRef; import akka.actor.Cancellable; import akka.actor.Props; -import akka.japi.Creator; import akka.japi.Option; import akka.japi.Pair; import com.google.common.base.Preconditions; @@ -27,7 +26,6 @@ import org.opendaylight.controller.remote.rpc.registry.RpcRegistry.Messages.Remo import org.opendaylight.controller.remote.rpc.registry.RpcRegistry.Messages.SetLocalRouter; import org.opendaylight.controller.remote.rpc.registry.gossip.Bucket; import org.opendaylight.controller.remote.rpc.registry.gossip.BucketStore; -import org.opendaylight.controller.remote.rpc.registry.mbeans.RemoteRpcRegistryMXBeanImpl; import org.opendaylight.controller.sal.connector.api.RpcRouter; import org.opendaylight.controller.sal.connector.api.RpcRouter.RouteIdentifier; import scala.concurrent.duration.FiniteDuration; @@ -50,7 +48,7 @@ public class RpcRegistry extends BucketStore { } public static Props props(RemoteRpcProviderConfig config) { - return Props.create(new RpcRegistryCreator(config)); + return Props.create(RpcRegistry.class, config); } @Override @@ -285,20 +283,4 @@ public class RpcRegistry extends BucketStore { } } } - - private static class RpcRegistryCreator implements Creator { - private static final long serialVersionUID = 1L; - private final RemoteRpcProviderConfig config; - - private RpcRegistryCreator(RemoteRpcProviderConfig config) { - this.config = config; - } - - @Override - public RpcRegistry create() throws Exception { - RpcRegistry registry = new RpcRegistry(config); - new RemoteRpcRegistryMXBeanImpl(registry); - return registry; - } - } } diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/gossip/Gossiper.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/gossip/Gossiper.java index 439c131e3f..d26bda1c25 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/gossip/Gossiper.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/gossip/Gossiper.java @@ -390,14 +390,17 @@ public class Gossiper extends AbstractUntypedActorWithMetering { localIsNewer.removeAll(remoteVersions.keySet()); - for (Address address : remoteVersions.keySet()) { - - if (localVersions.get(address) == null || remoteVersions.get(address) == null) { + for (Map.Entry entry : remoteVersions.entrySet()) { + Address address = entry.getKey(); + Long remoteVersion = entry.getValue(); + Long localVersion = localVersions.get(address); + if (localVersion == null || remoteVersion == null) { continue; //this condition is taken care of by above diffs } - if (localVersions.get(address) < remoteVersions.get(address)) { + + if (localVersion < remoteVersion) { localIsOlder.add(address); - } else if (localVersions.get(address) > remoteVersions.get(address)) { + } else if (localVersion > remoteVersion) { localIsNewer.add(address); } } diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/gossip/Messages.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/gossip/Messages.java index 99a94e70e9..4f18584399 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/gossip/Messages.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/gossip/Messages.java @@ -69,7 +69,7 @@ public class Messages { } } - public static class GetAllBucketsReply> extends ContainsBuckets implements Serializable { + public static class GetAllBucketsReply> extends ContainsBuckets { private static final long serialVersionUID = 1L; public GetAllBucketsReply(Map> buckets) { @@ -77,8 +77,7 @@ public class Messages { } } - public static class GetBucketsByMembersReply> extends ContainsBuckets - implements Serializable { + public static class GetBucketsByMembersReply> extends ContainsBuckets { private static final long serialVersionUID = 1L; public GetBucketsByMembersReply(Map> buckets) { @@ -107,7 +106,7 @@ public class Messages { } - public static class GetBucketVersionsReply extends ContainsBucketVersions implements Serializable { + public static class GetBucketVersionsReply extends ContainsBucketVersions { private static final long serialVersionUID = 1L; public GetBucketVersionsReply(Map versions) { @@ -115,8 +114,7 @@ public class Messages { } } - public static class UpdateRemoteBuckets> extends ContainsBuckets - implements Serializable { + public static class UpdateRemoteBuckets> extends ContainsBuckets { private static final long serialVersionUID = 1L; public UpdateRemoteBuckets(Map> buckets) { @@ -134,7 +132,7 @@ public class Messages { private static final long serialVersionUID = 5803354404380026143L; } - public static final class GossipStatus extends ContainsBucketVersions implements Serializable { + public static final class GossipStatus extends ContainsBucketVersions { private static final long serialVersionUID = -593037395143883265L; private final Address from; @@ -149,8 +147,7 @@ public class Messages { } } - public static final class GossipEnvelope> extends ContainsBuckets - implements Serializable { + public static final class GossipEnvelope> extends ContainsBuckets { private static final long serialVersionUID = 8346634072582438818L; private final Address from; diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/mbeans/RemoteRpcRegistryMXBeanImpl.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/mbeans/RemoteRpcRegistryMXBeanImpl.java index c0bdbb8d21..5fbd91cc81 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/mbeans/RemoteRpcRegistryMXBeanImpl.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/registry/mbeans/RemoteRpcRegistryMXBeanImpl.java @@ -12,6 +12,7 @@ import akka.actor.Address; import java.util.HashMap; import java.util.HashSet; import java.util.Map; +import java.util.Map.Entry; import java.util.Set; import org.opendaylight.controller.md.sal.common.util.jmx.AbstractMXBean; import org.opendaylight.controller.remote.rpc.registry.RoutingTable; @@ -81,9 +82,9 @@ public class RemoteRpcRegistryMXBeanImpl extends AbstractMXBean implements Remot // Get all RPCs from remote bucket Map> buckets = rpcRegistry.getRemoteBuckets(); - for (Address address : buckets.keySet()) { - RoutingTable table = buckets.get(address).getData(); - rpcMap.putAll(getRpcMemberMapByName(table, name, address.toString())); + for (Entry> entry : buckets.entrySet()) { + RoutingTable table = entry.getValue().getData(); + rpcMap.putAll(getRpcMemberMapByName(table, name, entry.getKey().toString())); } log.debug("list of RPCs {} searched by name {}", rpcMap, name); @@ -96,10 +97,9 @@ public class RemoteRpcRegistryMXBeanImpl extends AbstractMXBean implements Remot Map rpcMap = new HashMap<>(getRpcMemberMapByRoute(localTable, routeId, LOCAL_CONSTANT)); Map> buckets = rpcRegistry.getRemoteBuckets(); - for (Address address : buckets.keySet()) { - RoutingTable table = buckets.get(address).getData(); - rpcMap.putAll(getRpcMemberMapByRoute(table, routeId, address.toString())); - + for (Entry> entry : buckets.entrySet()) { + RoutingTable table = entry.getValue().getData(); + rpcMap.putAll(getRpcMemberMapByRoute(table, routeId, entry.getKey().toString())); } log.debug("list of RPCs {} searched by route {}", rpcMap, routeId); diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/utils/LatestEntryRoutingLogic.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/utils/LatestEntryRoutingLogic.java index c0e2973aab..f7b36a776e 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/utils/LatestEntryRoutingLogic.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/utils/LatestEntryRoutingLogic.java @@ -11,6 +11,7 @@ package org.opendaylight.controller.remote.rpc.utils; import akka.actor.ActorRef; import akka.japi.Pair; import com.google.common.base.Preconditions; +import java.io.Serializable; import java.util.Collection; import java.util.Comparator; import java.util.SortedSet; @@ -36,22 +37,28 @@ public class LatestEntryRoutingLogic implements RoutingLogic { return actorRefSet.last().first(); } - private class LatestEntryComparator implements Comparator> { + private static class LatestEntryComparator implements Comparator>, Serializable { + private static final long serialVersionUID = 1L; @Override public int compare(Pair o1, Pair o2) { if (o1 == null && o2 == null) { return 0; } - if (o1 == null && o2 != null) { + + if (o1 != null && o2 != null && o1.second() == null && o2.second() == null) { + return 0; + } + + if ((o1 == null || o1.second() == null) && o2 != null) { return -1; } - if (o1 != null && o2 == null) { + + if (o2 == null || o2.second() == null) { return 1; } return o1.second().compareTo(o2.second()); - } } } -- 2.36.6