PR feedback, move the actor builder closer to desired

Author: BrandonArp

src/main/java/com/arpnetworking/akka/ActorBuilder.java

@@ -15,25 +15,29 @@
  */
 package com.arpnetworking.akka;
 
-import akka.actor.Props;
+import akka.actor.Actor;
+import akka.japi.Creator;
 import com.arpnetworking.commons.builder.OvalBuilder;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 import java.util.function.Function;
 
 /**
  * Builder for actors.
  *
  * @param <B> The type of the builder
+ * @param <S> type of the object to be built
  * @author Brandon Arp (brandon dot arp at inscopemetrics dot com)
  */
-public abstract class ActorBuilder<B extends ActorBuilder<B>> extends OvalBuilder<Props> {
+@SuppressFBWarnings("SE_NO_SUITABLE_CONSTRUCTOR")
+public abstract class ActorBuilder<B extends ActorBuilder<B, S>, S extends Actor> extends OvalBuilder<S> implements Creator<S> {
     /**
-     * Protected constructor.
+     * Protected constructor for subclasses.
      *
-     * @param createProps method to create a {@link Props} from the {@link ActorBuilder}
+     * @param targetConstructor The constructor for the concrete type to be created by this builder.
      */
-    protected ActorBuilder(final Function<B, Props> createProps) {
-        super(createProps);
+    protected ActorBuilder(final Function<B, S> targetConstructor) {
+        super(targetConstructor);
     }
 
     /**
@@ -43,4 +47,14 @@ protected ActorBuilder(final Function<B, Props> createProps) {
      * @return instance with correct {@link ActorBuilder} class type.
      */
     protected abstract B self();
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public S create() throws Exception {
+        return build();
+    }
+
+    private static final long serialVersionUID = 1L;
 }

src/main/java/com/arpnetworking/akka/NonJoiningClusterJoiner.java

@@ -15,7 +15,6 @@
  */
 package com.arpnetworking.akka;
 
-import akka.actor.Props;
 import akka.actor.UntypedActor;
 import com.arpnetworking.steno.Logger;
 import com.arpnetworking.steno.LoggerFactory;
@@ -27,56 +26,32 @@
  */
 public final class NonJoiningClusterJoiner extends UntypedActor {
     /**
-     * Static factory method for creating a {@link Props} to create a {@link NonJoiningClusterJoiner} actor.
-     *
-     * @return a new {@link Props}
-     */
-    public static Props props() {
-        return Props.create(NonJoiningClusterJoiner.class);
-    }
-
-    /**
-     * Static factory method for creating a {@link Props} to create a {@link NonJoiningClusterJoiner} actor from a
-     * {@link Builder}.
-     *
-     * @param builder Builder to create the Props from
-     * @return a new {@link Props}
+     * {@inheritDoc}
      */
-    private static Props props(final Builder builder) {
-        return props();
+    @Override
+    public void onReceive(final Object message) throws Exception {
+        unhandled(message);
     }
 
-    /**
-     * Public constructor.
-     */
-    public NonJoiningClusterJoiner() {
+    private NonJoiningClusterJoiner(final Builder builder) {
         LOGGER.info()
                 .setMessage("NonJoiningClusterJoiner starting up")
                 .log();
     }
 
-
-    /**
-     * {@inheritDoc}
-     */
-    @Override
-    public void onReceive(final Object message) throws Exception {
-        unhandled(message);
-    }
-
     private static final Logger LOGGER = LoggerFactory.getLogger(NonJoiningClusterJoiner.class);
 
     /**
      * Implementation of the {@link com.arpnetworking.commons.builder.Builder} pattern for a {@link NonJoiningClusterJoiner}.
      *
      * @author Brandon Arp (brandon dot arp at inscopemetrics dot com)
      */
-    public static class Builder extends ActorBuilder<Builder> {
+    public static class Builder extends ActorBuilder<Builder, NonJoiningClusterJoiner> {
         /**
          * Public constructor.
          */
         public Builder() {
-            super(NonJoiningClusterJoiner::props);
+            super(NonJoiningClusterJoiner::new);
         }
 
         /**
@@ -86,5 +61,7 @@ public Builder() {
         public Builder self() {
             return this;
         }
+
+        private static final long serialVersionUID = 1L;
     }
 }

src/main/java/com/arpnetworking/clusteraggregator/GuiceModule.java

@@ -249,7 +249,7 @@ private ActorRef provideTcpServer(final Injector injector, final ActorSystem sys
     @Named("cluster-joiner")
     @SuppressFBWarnings("UPM_UNCALLED_PRIVATE_METHOD") // Invoked reflectively by Guice
     private ActorRef provideClusterJoiner(final ActorSystem system, final ClusterAggregatorConfiguration config) {
-        return system.actorOf(config.getClusterJoinActor(), "cluster-joiner");
+        return system.actorOf(Props.create(config.getClusterJoinActor()), "cluster-joiner");
     }
 
     @Provides

src/main/java/com/arpnetworking/clusteraggregator/configuration/ClusterAggregatorConfiguration.java

@@ -15,7 +15,8 @@
  */
 package com.arpnetworking.clusteraggregator.configuration;
 
-import akka.actor.Props;
+import akka.actor.Actor;
+import com.arpnetworking.akka.ActorBuilder;
 import com.arpnetworking.akka.NonJoiningClusterJoiner;
 import com.arpnetworking.commons.builder.OvalBuilder;
 import com.arpnetworking.commons.jackson.databind.ObjectMapperFactory;
@@ -49,7 +50,7 @@ public final class ClusterAggregatorConfiguration {
     public static ObjectMapper createObjectMapper() {
         final ObjectMapper objectMapper = ObjectMapperFactory.createInstance();
         final SimpleModule module = new SimpleModule();
-        module.addDeserializer(Props.class, new ActorBuilderDeserializer(objectMapper));
+        module.addDeserializer(ActorBuilder.class, new ActorBuilderDeserializer(objectMapper));
         objectMapper.registerModule(module);
         return objectMapper;
     }
@@ -122,7 +123,7 @@ public String getClusterHostSuffix() {
         return _clusterHostSuffix;
     }
 
-    public Props getClusterJoinActor() {
+    public ActorBuilder<?, ? extends Actor> getClusterJoinActor() {
         return _clusterJoinActor;
     }
 
@@ -191,7 +192,7 @@ private ClusterAggregatorConfiguration(final Builder builder) {
     private final Period _jvmMetricsCollectionInterval;
     private final RebalanceConfiguration _rebalanceConfiguration;
     private final String _clusterHostSuffix;
-    private final Props _clusterJoinActor;
+    private final ActorBuilder<?, ? extends Actor> _clusterJoinActor;
     private final Map<String, DatabaseConfiguration> _databaseConfigurations;
 
     private static final InterfaceDatabase INTERFACE_DATABASE = ReflectionsDatabase.newInstance();
@@ -411,7 +412,7 @@ public Builder setDatabaseConfigurations(final Map<String, DatabaseConfiguration
          * @param value The cluster join actor configuration.
          * @return This instance of <code>Builder</code>.
          */
-        public Builder setClusterJoinActor(final Props value) {
+        public Builder setClusterJoinActor(final ActorBuilder<?, ? extends Actor> value) {
             _clusterJoinActor = value;
             return this;
         }
@@ -456,7 +457,7 @@ public Builder setClusterJoinActor(final Props value) {
         @NotNull
         private String _clusterHostSuffix = "";
         @NotNull
-        private Props _clusterJoinActor = new NonJoiningClusterJoiner.Builder().build();
+        private ActorBuilder<?, ? extends Actor> _clusterJoinActor = new NonJoiningClusterJoiner.Builder();
         private Map<String, DatabaseConfiguration> _databaseConfigurations;
     }
 }

src/main/java/com/arpnetworking/configuration/jackson/akka/ActorBuilderDeserializer.java

@@ -15,7 +15,8 @@
  */
 package com.arpnetworking.configuration.jackson.akka;
 
-import akka.actor.Props;
+import akka.actor.Actor;
+import com.arpnetworking.akka.ActorBuilder;
 import com.arpnetworking.commons.builder.Builder;
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.TreeNode;
@@ -32,7 +33,7 @@
  *
  * @author Brandon Arp (brandon dot arp at inscopemetrics dot com)
  */
-public class ActorBuilderDeserializer extends JsonDeserializer<Props> {
+public class ActorBuilderDeserializer extends JsonDeserializer<ActorBuilder<?, ?>> {
     /**
      * Public constructor.
      *
@@ -43,23 +44,23 @@ public ActorBuilderDeserializer(final ObjectMapper mapper) {
     }
 
     @Override
-    public Props deserialize(final JsonParser p, final DeserializationContext ctxt) throws IOException {
+    public ActorBuilder<? extends ActorBuilder<?, ?>, ? extends Actor> deserialize(final JsonParser p, final DeserializationContext ctxt)
+            throws IOException {
         final TreeNode treeNode = p.readValueAsTree();
         final String type = ((TextNode) treeNode.get("type")).textValue();
         try {
             final Class<?> clazz = Class.forName(type);
-            final Class<? extends Builder<? extends Props>> builder = getBuilderForClass(clazz);
-            final Builder<? extends Props> value = _mapper.readValue(treeNode.traverse(), builder);
-            return value.build();
+            final Class<? extends ActorBuilder<? extends ActorBuilder<?, ?>, ? extends Actor>> builder = getBuilderForClass(clazz);
+            return _mapper.readValue(treeNode.traverse(), builder);
         } catch (final ClassNotFoundException e) {
             throw new JsonMappingException(p, String.format("Unable to find class %s referenced by Props type", type));
         }
     }
 
     @SuppressWarnings("unchecked")
-    private static Class<? extends Builder<Props>> getBuilderForClass(final Class<?> clazz)
+    private static Class<? extends ActorBuilder<? extends ActorBuilder<?, ?>, ? extends Actor>> getBuilderForClass(final Class<?> clazz)
             throws ClassNotFoundException {
-        return (Class<? extends Builder<Props>>) (Class.forName(
+        return (Class<? extends ActorBuilder<? extends ActorBuilder<?, ?>, ? extends Actor>>) (Class.forName(
                 clazz.getName() + "$Builder",
                 true, // initialize
                 clazz.getClassLoader()));

src/test/java/com/arpnetworking/clusteraggregator/configuration/ActorBuilderTest.java

@@ -15,7 +15,10 @@
  */
 package com.arpnetworking.clusteraggregator.configuration;
 
+import akka.actor.ActorRef;
+import akka.actor.PoisonPill;
 import akka.actor.Props;
+import com.arpnetworking.akka.ActorBuilder;
 import com.arpnetworking.akka.NonJoiningClusterJoiner;
 import com.arpnetworking.commons.jackson.databind.ObjectMapperFactory;
 import com.arpnetworking.configuration.jackson.akka.ActorBuilderDeserializer;
@@ -34,19 +37,23 @@ public class ActorBuilderTest extends BaseActorTest {
     @Test
     public void testBuild() {
         final NonJoiningClusterJoiner.Builder builder = new NonJoiningClusterJoiner.Builder();
-        builder.build();
+
+        final ActorRef ref = getSystem().actorOf(Props.create(builder));
+        ref.tell(PoisonPill.getInstance(), ActorRef.noSender());
     }
 
     @Test
     public void testPolyDeserialize() throws IOException {
         final ObjectMapper mapper = ObjectMapperFactory.createInstance();
         final SimpleModule module = new SimpleModule();
-        module.addDeserializer(Props.class, new ActorBuilderDeserializer(mapper));
+        module.addDeserializer(ActorBuilder.class, new ActorBuilderDeserializer(mapper));
         mapper.registerModule(module);
 
         @Language("JSON") final String data = "{\n"
                 + "  \"type\": \"com.arpnetworking.akka.NonJoiningClusterJoiner\"\n"
                 + "}";
-        final Props props = mapper.readValue(data, Props.class);
+        final ActorBuilder<?, ?> builder = mapper.readValue(data, ActorBuilder.class);
+        final ActorRef ref = getSystem().actorOf(Props.create(builder));
+        ref.tell(PoisonPill.getInstance(), ActorRef.noSender());
     }
 }