From: Robert Varga Date: Thu, 11 Oct 2018 16:42:48 +0000 (+0200) Subject: Enable mdsal-dom-broker spotbugs X-Git-Tag: v3.0.1~23 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=c918566080795a6f03e354c5bf1cc66c2f2ba364;p=mdsal.git Enable mdsal-dom-broker spotbugs This fixes up the issues reported and flips enforcement to on. Change-Id: Iae6de422f027e6413b4e3021d4324b051f41a513 Signed-off-by: Robert Varga --- diff --git a/dom/mdsal-dom-broker/pom.xml b/dom/mdsal-dom-broker/pom.xml index 2ef75c0ad4..15b61d90c0 100644 --- a/dom/mdsal-dom-broker/pom.xml +++ b/dom/mdsal-dom-broker/pom.xml @@ -79,6 +79,13 @@ checkstyle.violationSeverity=error + + com.github.spotbugs + spotbugs-maven-plugin + + true + + diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/AbstractDOMDataBroker.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/AbstractDOMDataBroker.java index 95ffcfc5a7..e0b571f1e6 100644 --- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/AbstractDOMDataBroker.java +++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/AbstractDOMDataBroker.java @@ -30,7 +30,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; public abstract class AbstractDOMDataBroker extends AbstractDOMForwardedTransactionFactory - implements DOMDataBroker, AutoCloseable { + implements DOMDataBroker { private static final Logger LOG = LoggerFactory.getLogger(AbstractDOMDataBroker.class); private final AtomicLong txNum = new AtomicLong(); diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/CommitCoordinationTask.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/CommitCoordinationTask.java index d243a4da1e..262b0b35dd 100644 --- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/CommitCoordinationTask.java +++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/CommitCoordinationTask.java @@ -5,13 +5,13 @@ * terms of the Eclipse Public License v1.0 which accompanies this distribution, * and is available at http://www.eclipse.org/legal/epl-v10.html */ - package org.opendaylight.mdsal.dom.broker; import com.google.common.base.Preconditions; import com.google.common.base.Throwables; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.util.Collection; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; @@ -24,8 +24,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Implementation of blocking three-phase commit-coordination tasks without - * support of cancellation. + * Implementation of blocking three-phase commit-coordination tasks without support of cancellation. */ final class CommitCoordinationTask implements Callable { private enum Phase { @@ -79,17 +78,15 @@ final class CommitCoordinationTask implements Callable { } /** - * Invokes canCommit on underlying cohorts and blocks till - * all results are returned. - * - *

- * Valid state transition is from SUBMITTED to CAN_COMMIT, - * if currentPhase is not SUBMITTED throws IllegalStateException. + * Invokes canCommit on underlying cohorts and blocks till all results are returned. * - * @throws TransactionCommitFailedException - * If one of cohorts failed can Commit + *

+ * Valid state transition is from SUBMITTED to CAN_COMMIT, if currentPhase is not SUBMITTED throws + * IllegalStateException. * + * @throws TransactionCommitFailedException If one of cohorts failed can Commit */ + @SuppressFBWarnings("BC_UNCONFIRMED_CAST_OF_RETURN_VALUE") private void canCommitBlocking() throws TransactionCommitFailedException { for (final ListenableFuture canCommit : canCommitAll()) { try { @@ -104,16 +101,14 @@ final class CommitCoordinationTask implements Callable { } /** - * Invokes canCommit on underlying cohorts and returns composite future - * which will contains {@link Boolean#TRUE} only and only if - * all cohorts returned true. + * Invokes canCommit on underlying cohorts and returns composite future which will contain {@link Boolean#TRUE} only + * and only if all cohorts returned true. * - *

- * Valid state transition is from SUBMITTED to CAN_COMMIT, - * if currentPhase is not SUBMITTED throws IllegalStateException. + *

+ * Valid state transition is from SUBMITTED to CAN_COMMIT, if currentPhase is not SUBMITTED throws + * IllegalStateException. * * @return List of all cohorts futures from can commit phase. - * */ private ListenableFuture[] canCommitAll() { final ListenableFuture[] ops = new ListenableFuture[cohorts.size()]; @@ -125,18 +120,15 @@ final class CommitCoordinationTask implements Callable { } /** - * Invokes preCommit on underlying cohorts and blocks till - * all results are returned. + * Invokes preCommit on underlying cohorts and blocks until all results are returned. * - *

- * Valid state transition is from CAN_COMMIT to PRE_COMMIT, if current - * state is not CAN_COMMIT - * throws IllegalStateException. - * - * @throws TransactionCommitFailedException - * If one of cohorts failed preCommit + *

+ * Valid state transition is from CAN_COMMIT to PRE_COMMIT, if current state is not CAN_COMMIT throws + * IllegalStateException. * + * @throws TransactionCommitFailedException If one of cohorts failed preCommit */ + @SuppressFBWarnings("BC_UNCONFIRMED_CAST_OF_RETURN_VALUE") private void preCommitBlocking() throws TransactionCommitFailedException { final ListenableFuture[] preCommitFutures = preCommitAll(); try { @@ -149,17 +141,14 @@ final class CommitCoordinationTask implements Callable { } /** - * Invokes preCommit on underlying cohorts and returns future - * which will complete once all preCommit on cohorts completed or - * failed. + * Invokes preCommit on underlying cohorts and returns future which will complete once all preCommit on cohorts + * completed or failed. * - *

- * Valid state transition is from CAN_COMMIT to PRE_COMMIT, if current - * state is not CAN_COMMIT - * throws IllegalStateException. + *

+ * Valid state transition is from CAN_COMMIT to PRE_COMMIT, if current state is not CAN_COMMIT throws + * IllegalStateException. * * @return List of all cohorts futures from can commit phase. - * */ private ListenableFuture[] preCommitAll() { final ListenableFuture[] ops = new ListenableFuture[cohorts.size()]; @@ -171,17 +160,14 @@ final class CommitCoordinationTask implements Callable { } /** - * Invokes commit on underlying cohorts and blocks till - * all results are returned. + * Invokes commit on underlying cohorts and blocks until all results are returned. * - *

- * Valid state transition is from PRE_COMMIT to COMMIT, if not throws - * IllegalStateException. - * - * @throws TransactionCommitFailedException - * If one of cohorts failed preCommit + *

+ * Valid state transition is from PRE_COMMIT to COMMIT, if not throws IllegalStateException. * + * @throws TransactionCommitFailedException If one of cohorts failed preCommit */ + @SuppressFBWarnings("BC_UNCONFIRMED_CAST_OF_RETURN_VALUE") private void commitBlocking() throws TransactionCommitFailedException { final ListenableFuture[] commitFutures = commitAll(); try { @@ -216,26 +202,18 @@ final class CommitCoordinationTask implements Callable { /** * Aborts transaction. * - *

- * Invokes {@link DOMStoreThreePhaseCommitCohort#abort()} on all - * cohorts, blocks - * for all results. If any of the abort failed throws - * IllegalStateException, - * which will contains originalCause as suppressed Exception. + *

+ * Invokes {@link DOMStoreThreePhaseCommitCohort#abort()} on all cohorts, blocks for all results. If any + * of the abort failed throws IllegalStateException, which will contains originalCause as suppressed Exception. * - *

+ *

* If aborts we're successful throws supplied exception * - * @param originalCause - * Exception which should be used to fail transaction for - * consumers of transaction - * future and listeners of transaction failure. + * @param originalCause Exception which should be used to fail transaction for consumers of transaction future + * and listeners of transaction failure. * @param phase phase in which the problem ensued - * @throws TransactionCommitFailedException - * on invocation of this method. - * originalCa - * @throws IllegalStateException - * if abort failed. + * @throws TransactionCommitFailedException on invocation of this method. + * @throws IllegalStateException if abort failed. */ private void abortBlocking(final TransactionCommitFailedException originalCause) throws TransactionCommitFailedException { @@ -251,11 +229,9 @@ final class CommitCoordinationTask implements Callable { } /** - * Invokes abort on underlying cohorts and returns future which - * completes once all abort on cohorts are completed. + * Invokes abort on underlying cohorts and returns future which completes once all abort on cohorts are completed. * - * @return Future which will complete once all cohorts completed - * abort. + * @return Future which will complete once all cohorts completed abort. */ @SuppressWarnings({"unchecked", "rawtypes"}) private ListenableFuture abortAsyncAll() { diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouter.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouter.java index e8a307c958..f84ef6699e 100644 --- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouter.java +++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMNotificationRouter.java @@ -45,7 +45,6 @@ import org.opendaylight.yangtools.yang.model.api.SchemaPath; import org.slf4j.Logger; import org.slf4j.LoggerFactory; - /** * Joint implementation of {@link DOMNotificationPublishService} and {@link DOMNotificationService}. Provides * routing of notifications from publishers to subscribers. @@ -84,7 +83,6 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl ListenerRegistry.create(); private final ScheduledThreadPoolExecutor observer; - @SuppressWarnings("unchecked") @VisibleForTesting DOMNotificationRouter(final ExecutorService executor, final int queueDepth, final WaitStrategy strategy) { this.executor = Preconditions.checkNotNull(executor); @@ -162,7 +160,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl private void notifyListenerTypesChanged(final Set typesAfter) { final List> listenersAfter = ImmutableList.copyOf(subscriptionListeners.getListeners()); - executor.submit(() -> { + executor.execute(() -> { for (final ListenerRegistration subListener : listenersAfter) { try { subListener.getInstance().onSubscriptionChanged(typesAfter); @@ -177,7 +175,7 @@ public class DOMNotificationRouter implements AutoCloseable, DOMNotificationPubl public ListenerRegistration registerSubscriptionListener( final L listener) { final Set initialTypes = listeners.keySet(); - executor.submit(() -> listener.onSubscriptionChanged(initialTypes)); + executor.execute(() -> listener.onSubscriptionChanged(initialTypes)); return subscriptionListeners.registerWithType(listener); } diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouter.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouter.java index 317009d71e..f0215481c6 100644 --- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouter.java +++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/DOMRpcRouter.java @@ -184,7 +184,7 @@ public final class DOMRpcRouter extends AbstractRegistration implements SchemaCo } @VisibleForTesting - Collection listeners() { + synchronized Collection listeners() { return listeners; } diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTree.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTree.java index 804beedb6a..71e63d8094 100644 --- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTree.java +++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMDataTree.java @@ -7,8 +7,10 @@ */ package org.opendaylight.mdsal.dom.broker; -import com.google.common.base.Preconditions; -import com.google.common.base.Verify; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Verify.verifyNotNull; +import static java.util.Objects.requireNonNull; + import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ClassToInstanceMap; import com.google.common.collect.ImmutableClassToInstanceMap; @@ -72,12 +74,13 @@ public final class ShardedDOMDataTree implements DOMDataTreeService, DOMDataTree public DOMDataTreeShardRegistration registerDataTreeShard( final DOMDataTreeIdentifier prefix, final T shard, final DOMDataTreeProducer producer) throws DOMDataTreeShardingConflictException { + checkArgument(producer instanceof ShardedDOMDataTreeProducer, "Unsupported producer %s", producer); + final ShardedDOMDataTreeProducer prod = (ShardedDOMDataTreeProducer) producer; - final DOMDataTreeIdentifier firstSubtree = Iterables.getOnlyElement((( - ShardedDOMDataTreeProducer) producer).getSubtrees()); - Preconditions.checkArgument(firstSubtree != null, "Producer that is used to verify namespace claim can" + final DOMDataTreeIdentifier firstSubtree = Iterables.getOnlyElement(prod.getSubtrees()); + checkArgument(firstSubtree != null, "Producer that is used to verify namespace claim can" + " only claim a single namespace"); - Preconditions.checkArgument(prefix.equals(firstSubtree), "Trying to register shard to a different namespace" + checkArgument(prefix.equals(firstSubtree), "Trying to register shard to a different namespace" + " than the producer has claimed"); final DOMDataTreeShardRegistration reg; @@ -106,7 +109,7 @@ public final class ShardedDOMDataTree implements DOMDataTreeService, DOMDataTree shards.store(prefix, reg); - ((ShardedDOMDataTreeProducer) producer).subshardAdded(Collections.singletonMap(prefix, shard)); + prod.subshardAdded(Collections.singletonMap(prefix, shard)); } // Notify the parent shard @@ -152,13 +155,13 @@ public final class ShardedDOMDataTree implements DOMDataTreeService, DOMDataTree @Override public synchronized DOMDataTreeProducer createProducer(final Collection subtrees) { - Preconditions.checkArgument(!subtrees.isEmpty(), "Subtrees may not be empty"); + checkArgument(!subtrees.isEmpty(), "Subtrees may not be empty"); final Map shardMap = new HashMap<>(); for (final DOMDataTreeIdentifier subtree : subtrees) { // Attempting to create a disconnected producer -- all subtrees have to be unclaimed final DOMDataTreeProducer producer = findProducer(subtree); - Preconditions.checkArgument(producer == null, "Subtree %s is attached to producer %s", subtree, producer); + checkArgument(producer == null, "Subtree %s is attached to producer %s", subtree, producer); final DOMDataTreePrefixTableEntry> possibleShardReg = shards.lookup(subtree); @@ -172,7 +175,7 @@ public final class ShardedDOMDataTree implements DOMDataTreeService, DOMDataTree synchronized DOMDataTreeProducer createProducer(final ShardedDOMDataTreeProducer parent, final Collection subtrees) { - Preconditions.checkNotNull(parent); + requireNonNull(parent); final Map shardMap = new HashMap<>(); for (final DOMDataTreeIdentifier s : subtrees) { @@ -187,7 +190,7 @@ public final class ShardedDOMDataTree implements DOMDataTreeService, DOMDataTree public synchronized ListenerRegistration registerListener(final T listener, final Collection subtrees, final boolean allowRxMerges, final Collection producers) throws DOMDataTreeLoopException { - Preconditions.checkNotNull(listener, "listener"); + requireNonNull(listener, "listener"); // Cross-check specified trees for exclusivity and eliminate duplicates, noDupSubtrees is effectively a Set final Collection noDupSubtrees; @@ -202,8 +205,8 @@ public final class ShardedDOMDataTree implements DOMDataTreeService, DOMDataTree for (DOMDataTreeIdentifier toCheck : subtrees) { for (DOMDataTreeIdentifier against : subtrees) { if (!toCheck.equals(against)) { - Preconditions.checkArgument(!toCheck.contains(against), "Subtree %s contains subtree %s", - toCheck, against); + checkArgument(!toCheck.contains(against), "Subtree %s contains subtree %s", toCheck, + against); } } } @@ -217,7 +220,7 @@ public final class ShardedDOMDataTree implements DOMDataTreeService, DOMDataTree final ListMultimap, DOMDataTreeIdentifier> needed = ArrayListMultimap.create(); for (final DOMDataTreeIdentifier subtree : subtrees) { - final DOMDataTreeShardRegistration reg = Verify.verifyNotNull(shards.lookup(subtree).getValue()); + final DOMDataTreeShardRegistration reg = verifyNotNull(shards.lookup(subtree).getValue()); needed.put(reg, subtree); } @@ -226,14 +229,14 @@ public final class ShardedDOMDataTree implements DOMDataTreeService, DOMDataTree // Sanity check: all selected shards have to support one of the listening interfaces needed.asMap().forEach((reg, trees) -> { final DOMDataTreeShard shard = reg.getInstance(); - Preconditions.checkArgument(shard instanceof ListenableDOMDataTreeShard + checkArgument(shard instanceof ListenableDOMDataTreeShard || shard instanceof DOMStoreTreeChangePublisher, "Subtrees %s do not point to listenable subtree.", trees); }); // Sanity check: all producers have to come from this implementation and must not form loops for (DOMDataTreeProducer producer : producers) { - Preconditions.checkArgument(producer instanceof ShardedDOMDataTreeProducer); + checkArgument(producer instanceof ShardedDOMDataTreeProducer); simpleLoopCheck(subtrees, ((ShardedDOMDataTreeProducer) producer).getSubtrees()); } diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMWriteTransactionAdapter.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMWriteTransactionAdapter.java index 594de1565b..063becf8f0 100644 --- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMWriteTransactionAdapter.java +++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/ShardedDOMWriteTransactionAdapter.java @@ -101,7 +101,8 @@ public class ShardedDOMWriteTransactionAdapter implements DOMDataTreeWriteTransa public void put(final LogicalDatastoreType store, final YangInstanceIdentifier path, final NormalizedNode data) { checkRunning(); - LOG.debug("{}: Invoking put operation at {}:{} with payload {}", txIdentifier, store, path); + LOG.debug("{}: Invoking put operation at {}:{}", txIdentifier, store, path); + LOG.trace("{}: payload is {}", txIdentifier, data); if (!initialized) { initializeDataTreeProducerLayer(path.getParent()); } @@ -114,7 +115,8 @@ public class ShardedDOMWriteTransactionAdapter implements DOMDataTreeWriteTransa public void merge(final LogicalDatastoreType store, final YangInstanceIdentifier path, final NormalizedNode data) { checkRunning(); - LOG.debug("{}: Invoking merge operation at {}:{} with payload {}", txIdentifier, store, path); + LOG.debug("{}: Invoking merge operation at {}:{}", txIdentifier, store, path); + LOG.trace("{}: payload is {}", txIdentifier, data); if (!initialized) { initializeDataTreeProducerLayer(path.getParent()); } @@ -150,7 +152,7 @@ public class ShardedDOMWriteTransactionAdapter implements DOMDataTreeWriteTransa treeService.createProducer( Collections.singleton(new DOMDataTreeIdentifier(LogicalDatastoreType.OPERATIONAL, path)))); - LOG.debug("{}: Creating DOMDataTreeCursorAwareTransactions delegates", txIdentifier, path); + LOG.debug("{}: Creating DOMDataTreeCursorAwareTransactions delegates on {}", txIdentifier, path); transactionMap.put(LogicalDatastoreType.CONFIGURATION, producerMap.get(LogicalDatastoreType.CONFIGURATION).createTransaction(true)); transactionMap.put(LogicalDatastoreType.OPERATIONAL, diff --git a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/UnknownDOMRpcRoutingTableEntry.java b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/UnknownDOMRpcRoutingTableEntry.java index 13bd293251..eb172df890 100644 --- a/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/UnknownDOMRpcRoutingTableEntry.java +++ b/dom/mdsal-dom-broker/src/main/java/org/opendaylight/mdsal/dom/broker/UnknownDOMRpcRoutingTableEntry.java @@ -7,25 +7,17 @@ */ package org.opendaylight.mdsal.dom.broker; -import com.google.common.util.concurrent.FluentFuture; import java.util.List; import java.util.Map; import org.opendaylight.mdsal.dom.api.DOMRpcIdentifier; import org.opendaylight.mdsal.dom.api.DOMRpcImplementation; -import org.opendaylight.mdsal.dom.api.DOMRpcImplementationNotAvailableException; -import org.opendaylight.mdsal.dom.api.DOMRpcResult; -import org.opendaylight.yangtools.util.concurrent.FluentFutures; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; import org.opendaylight.yangtools.yang.model.api.SchemaPath; final class UnknownDOMRpcRoutingTableEntry extends AbstractDOMRpcRoutingTableEntry { - private final FluentFuture unknownRpc; - UnknownDOMRpcRoutingTableEntry(final SchemaPath schemaPath, final Map> impls) { super(DOMRpcIdentifier.create(schemaPath), impls); - unknownRpc = FluentFutures.immediateFailedFluentFuture( - new DOMRpcImplementationNotAvailableException("SchemaPath %s is not resolved to an RPC", schemaPath)); } @Override