Do not use Optional in InstallSnapshot

Passing an Optional in method arguments is an early-adopter antipattern,
as is storing it in the fields. Use a plain @Nullable, which makes it a
tad easier to use.

Change-Id: I1acb67d38bb2be561cbc77041a9a1e07ed004b88
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java
index 8085bea478002d31b52a67f0539005e3113e0e8c..44f84b7f9cdf4fe4833bb96142cad41345864707 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/behaviors/AbstractLeader.java
@@ -959,9 +959,9 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior {
                         nextSnapshotChunk.length);
 
                 int nextChunkIndex = installSnapshotState.incrementChunkIndex();
-                Optional<ServerConfigurationPayload> serverConfig = Optional.empty();
+                ServerConfigurationPayload serverConfig = null;
                 if (installSnapshotState.isLastChunk(nextChunkIndex)) {
-                    serverConfig = Optional.ofNullable(context.getPeerServerInfo(true));
+                    serverConfig = context.getPeerServerInfo(true);
                 }
 
                 sendSnapshotChunk(followerActor, followerLogInfo, nextSnapshotChunk, nextChunkIndex, serverConfig);
@@ -980,7 +980,7 @@ public abstract class AbstractLeader extends AbstractRaftActorBehavior {
 
     private void sendSnapshotChunk(final ActorSelection followerActor, final FollowerLogInformation followerLogInfo,
                                    final byte[] snapshotChunk, final int chunkIndex,
-                                   final Optional<ServerConfigurationPayload> serverConfig) {
+                                   final @Nullable ServerConfigurationPayload serverConfig) {
         LeaderInstallSnapshotState installSnapshotState = followerLogInfo.getInstallSnapshotState();
 
         installSnapshotState.startChunkTimer();

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 aa32de1ebbe111cf51655c1d58cb0e3eb25b0fab..e20d485166599e4e4869eb335cf3be03e75322db 100644 (file)
--- 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
@@ -651,7 +651,7 @@ public class Follower extends AbstractRaftActorBehavior {
             installSnapshot.getLastIncludedIndex(), installSnapshot.getLastIncludedTerm(),
             installSnapshot.getLastIncludedIndex(), installSnapshot.getLastIncludedTerm(),
             context.getTermInformation().getCurrentTerm(), context.getTermInformation().getVotedFor(),
-            installSnapshot.getServerConfig().orElse(null));
+            installSnapshot.serverConfig());
 
         final var applySnapshotCallback = new ApplySnapshot.Callback() {
             @Override

diff --git a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/IS.java b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/IS.java
index 3247bb241ecad0da01ce35e9f7836f2d0011fbdb..7113ad3fb977c1c82eeced8cfb5012b086183ca1 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/IS.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/main/java/org/opendaylight/controller/cluster/raft/messages/IS.java
@@ -14,7 +14,6 @@ import java.io.Externalizable;
 import java.io.IOException;
 import java.io.ObjectInput;
 import java.io.ObjectOutput;
-import java.util.Optional;
 import java.util.OptionalInt;
 import org.opendaylight.controller.cluster.raft.RaftVersions;
 import org.opendaylight.controller.cluster.raft.persisted.ServerConfigurationPayload;
@@ -49,8 +48,8 @@ final class IS implements Externalizable {
         if (lastChunkHashCode.isPresent()) {
             flags |= LAST_CHUNK_HASHCODE;
         }
-        final var serverConfig = installSnapshot.getServerConfig();
-        if (serverConfig.isPresent()) {
+        final var serverConfig = installSnapshot.serverConfig();
+        if (serverConfig != null) {
             flags |= SERVER_CONFIG;
         }
 
@@ -63,8 +62,8 @@ final class IS implements Externalizable {
         if (lastChunkHashCode.isPresent()) {
             out.writeInt(lastChunkHashCode.orElseThrow());
         }
-        if (serverConfig.isPresent()) {
-            out.writeObject(serverConfig.orElseThrow());
+        if (serverConfig != null) {
+            out.writeObject(serverConfig);
         }
 
         out.writeObject(installSnapshot.getData());
@@ -86,8 +85,8 @@ final class IS implements Externalizable {
 
         OptionalInt lastChunkHashCode = getFlag(flags, LAST_CHUNK_HASHCODE) ? OptionalInt.of(in.readInt())
             : OptionalInt.empty();
-        Optional<ServerConfigurationPayload> serverConfig = getFlag(flags, SERVER_CONFIG)
-                ? Optional.of((ServerConfigurationPayload)in.readObject()) : Optional.empty();
+        ServerConfigurationPayload serverConfig = getFlag(flags, SERVER_CONFIG)
+                ? requireNonNull((ServerConfigurationPayload) in.readObject()) : null;
 
         byte[] data = (byte[])in.readObject();
 

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 3cd470f6cc6d3c7186dc4fb20324482b216153d7..85ff4193a8cbbef1c2bf8941d5dc404dfbe5fe83 100644 (file)
--- 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
@@ -7,14 +7,16 @@
  */
 package org.opendaylight.controller.cluster.raft.messages;
 
+import static java.util.Objects.requireNonNull;
+
 import com.google.common.annotations.VisibleForTesting;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.Externalizable;
 import java.io.IOException;
 import java.io.ObjectInput;
 import java.io.ObjectOutput;
-import java.util.Optional;
 import java.util.OptionalInt;
+import org.eclipse.jdt.annotation.Nullable;
 import org.opendaylight.controller.cluster.raft.RaftVersions;
 import org.opendaylight.controller.cluster.raft.persisted.ServerConfigurationPayload;
 
@@ -33,8 +35,7 @@ public final class InstallSnapshot extends AbstractRaftRPC {
     private final int totalChunks;
     @SuppressFBWarnings(value = "SE_BAD_FIELD", justification = "Handled via writeReplace()")
     private final OptionalInt lastChunkHashCode;
-    @SuppressFBWarnings(value = "SE_BAD_FIELD", justification = "Handled via writeReplace()")
-    private final Optional<ServerConfigurationPayload> serverConfig;
+    private final @Nullable ServerConfigurationPayload serverConfig;
     private final short recipientRaftVersion;
 
     @SuppressFBWarnings(value = "EI_EXPOSE_REP2", justification = """
@@ -43,7 +44,7 @@ public final class InstallSnapshot extends AbstractRaftRPC {
         large.""")
     public InstallSnapshot(final long term, final String leaderId, final long lastIncludedIndex,
             final long lastIncludedTerm, final byte[] data, final int chunkIndex, final int totalChunks,
-            final OptionalInt lastChunkHashCode, final Optional<ServerConfigurationPayload> serverConfig,
+            final OptionalInt lastChunkHashCode, final @Nullable ServerConfigurationPayload serverConfig,
             final short recipientRaftVersion) {
         super(term);
         this.leaderId = leaderId;
@@ -62,7 +63,7 @@ public final class InstallSnapshot extends AbstractRaftRPC {
                            final long lastIncludedTerm, final byte[] data, final int chunkIndex,
                            final int totalChunks) {
         this(term, leaderId, lastIncludedIndex, lastIncludedTerm, data, chunkIndex, totalChunks, OptionalInt.empty(),
-            Optional.empty(), RaftVersions.CURRENT_VERSION);
+            null, RaftVersions.CURRENT_VERSION);
     }
 
     public String getLeaderId() {
@@ -97,7 +98,7 @@ public final class InstallSnapshot extends AbstractRaftRPC {
         return lastChunkHashCode;
     }
 
-    public Optional<ServerConfigurationPayload> getServerConfig() {
+    public @Nullable ServerConfigurationPayload serverConfig() {
         return serverConfig;
     }
 
@@ -106,7 +107,7 @@ public final class InstallSnapshot extends AbstractRaftRPC {
         return "InstallSnapshot [term=" + getTerm() + ", leaderId=" + leaderId + ", lastIncludedIndex="
                 + lastIncludedIndex + ", lastIncludedTerm=" + lastIncludedTerm + ", datasize=" + data.length
                 + ", Chunk=" + chunkIndex + "/" + totalChunks + ", lastChunkHashCode=" + lastChunkHashCode
-                + ", serverConfig=" + serverConfig.orElse(null) + "]";
+                + ", serverConfig=" + serverConfig + "]";
     }
 
     @Override
@@ -144,9 +145,9 @@ public final class InstallSnapshot extends AbstractRaftRPC {
                 out.writeInt(installSnapshot.lastChunkHashCode.orElseThrow());
             }
 
-            out.writeByte(installSnapshot.serverConfig.isPresent() ? 1 : 0);
-            if (installSnapshot.serverConfig.isPresent()) {
-                out.writeObject(installSnapshot.serverConfig.orElseThrow());
+            out.writeByte(installSnapshot.serverConfig != null ? 1 : 0);
+            if (installSnapshot.serverConfig != null) {
+                out.writeObject(installSnapshot.serverConfig);
             }
 
             out.writeObject(installSnapshot.data);
@@ -162,8 +163,12 @@ public final class InstallSnapshot extends AbstractRaftRPC {
             int totalChunks = in.readInt();
 
             OptionalInt lastChunkHashCode = in.readByte() == 1 ? OptionalInt.of(in.readInt()) : OptionalInt.empty();
-            Optional<ServerConfigurationPayload> serverConfig = in.readByte() == 1
-                    ? Optional.of((ServerConfigurationPayload)in.readObject()) : Optional.empty();
+            ServerConfigurationPayload serverConfig;
+            if (in.readByte() == 1) {
+                serverConfig = requireNonNull((ServerConfigurationPayload) in.readObject());
+            } else {
+                serverConfig = null;
+            }
 
             byte[] data = (byte[])in.readObject();
 

diff --git a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/messages/InstallSnapshotTest.java b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/messages/InstallSnapshotTest.java
index 090ab77dad7275cf68848bf80affc2dcafaa21fe..4e0b1bc0915bff2f315e21b86cfaad16f227f612 100644 (file)
--- a/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/messages/InstallSnapshotTest.java
+++ b/opendaylight/md-sal/sal-akka-raft/src/test/java/org/opendaylight/controller/cluster/raft/messages/InstallSnapshotTest.java
@@ -11,7 +11,6 @@ import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 
 import java.util.List;
-import java.util.Optional;
 import java.util.OptionalInt;
 import org.apache.commons.lang3.SerializationUtils;
 import org.junit.Test;
@@ -47,10 +46,10 @@ public class InstallSnapshotTest {
         var serverConfig = new ServerConfigurationPayload(List.of(
                 new ServerInfo("leader", true), new ServerInfo("follower", false)));
         assertInstallSnapshot(fullSize, new InstallSnapshot(3L, "leaderId", 11L, 2L, data, 5, 6, OptionalInt.of(54321),
-            Optional.of(serverConfig), raftVersion));
+            serverConfig, raftVersion));
 
         assertInstallSnapshot(emptySize, new InstallSnapshot(3L, "leaderId", 11L, 2L, data, 5, 6, OptionalInt.empty(),
-            Optional.empty(), raftVersion));
+            null, raftVersion));
     }
 
     private static void assertInstallSnapshot(final int expectedSize, final InstallSnapshot expected) {
@@ -76,11 +75,6 @@ public class InstallSnapshotTest {
                     actual.getLastChunkHashCode());
         }
 
-        assertEquals("getServerConfig present", expected.getServerConfig().isPresent(),
-                actual.getServerConfig().isPresent());
-        if (expected.getServerConfig().isPresent()) {
-            assertEquals("getServerConfig", expected.getServerConfig().orElseThrow().getServerConfig(),
-                    actual.getServerConfig().orElseThrow().getServerConfig());
-        }
+        assertEquals(expected.serverConfig(), actual.serverConfig());
     }
 }