Actor builder and updates #51

BrandonArp · 2016-12-19T21:23:51Z

Just a preview. I need to add the GraphiteSink back in with Akka actors. I might also pull out the updates from the ActorBuilder stuff.

BrandonArp · 2016-12-30T23:33:58Z

The GraphiteSink was updated and the updated dependencies are in a separate PR. Please take a look at #52 before this.

BrandonArp · 2017-01-09T19:05:38Z

Rebased off the updated dependencies. There are a few minor changes in here that are unrelated: minor refactor of emitter creation, changing to Java-based futures in Status actor.

vjkoskela · 2017-01-31T03:52:31Z

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

+ *
+ * @author Brandon Arp (brandon dot arp at inscopemetrics dot com)
+ */
+public class NonJoiningClusterJoiner extends UntypedActor {


Oh you and your final classes.

vjkoskela · 2017-01-31T03:53:12Z

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

+     * @param builder Builder to create the Props from
+     * @return a new {@link Props}
+     */
+    private static Props props(final Builder builder) {


Builder is unused and the method is private

The builder references the static method as the constructor for the Props. It looks similar to our other actors (except for the private part). I could make the props method public, but I'm not sure what that would really achieve.

I guess my confusion here is that the builder argument is unused and since it's static it can't be overwritten. This is also not the standard builder pattern so it's not invoked reflectively. Still a little confused here.

The Builder still references this method just like the constructor in the other Builders. The base builder takes a Function<Builder, Props>. Hence the need to have that signature.

vjkoskela · 2017-01-31T03:54:08Z

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

+     * Public constructor.
+     */
+    public NonJoiningClusterJoiner() {
+        LOGGER.info()


Shouldn't this go in the actor startup routine, not the ctor?

Eh, I could put it on the onStart. I've tended toward the constuctor, though.

Truth in advertising. It doesn't "start" on construction.

vjkoskela · 2017-01-31T03:54:24Z

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

+                .log();
+    }
+
+    @Override


Inherit doc

vjkoskela · 2017-01-31T03:54:52Z

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

+            super(NonJoiningClusterJoiner::props);
+        }
+
+        @Override


vjkoskela · 2017-01-31T03:56:15Z

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

+                    .thenApply(CAST_MAPPER);
+            stateFuture.whenComplete(
+                    (statusResponse, throwable) -> {
+                        final boolean healthy = _cluster.readView().self().status() == MemberStatus.up() && !_quarantined;


Objects.equal instead of ==

vjkoskela · 2017-01-31T03:58:17Z

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

+        try {
+            final Class<?> clazz = Class.forName(type);
+            final Class<? extends Builder<? extends Props>> builder = getBuilderForClass(clazz);
+            final Builder<? extends Props> value = _mapper.readValue(treeNode.toString(), builder);


You should be able to avoid the toString here

I think I found a suitable alternative. Docs indicate that it should be more efficient.

vjkoskela · 2017-01-31T04:01:51Z

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

+    @Override
+    public Props deserialize(final JsonParser p, final DeserializationContext ctxt) throws IOException {
+        final TreeNode treeNode = p.readValueAsTree();
+        final String type = ((TextNode) treeNode.get("type")).textValue();


This seems like a bit of reinvention. Type mapping is already supported by Jackson, I know we haven't used it, but I think so far we've avoided this level of generality. For example we have a getClass helper from config which we then use to deserialize but we never fetch the builder by hand. Could we just do something like that here?

I tried, but I couldn't pull it off. This was the best I was able to do. Everything else got.... interestingly broken. It had to do with the return type all being Props, but the deserializer not being a subclass of Props and the fact that each Props will have a different constructor. I don't think Jackson was setup to do that.

Hrm. Ok.

The use case seems to me be a combination of 1) Polymorphism; different builder types as determined by a field; 2) The use of a builder. In the first we've either done explicit type request via a TypeReference or annotation based binding via a type field.

Props is final so you can't wrap it or extend it in order to annotate it.

http://doc.akka.io/japi/akka/2.4.0/akka/actor/Props.html

The problem seems to be that we're storing any Props here for the _clusterJoinerActor which is not type-complete (I'm guess not just any actor can be a "joiner actor").

For example, if the Props were wrapped with NonJoiningClusterJoiner and we used that type here (or more correctly an interface of that type) then we would be just to a manageable polymorphic deserialization problem (e.g. annotate the subclasses of that interface). Using Props directly corners us in many ways, and doesn't help describe what is actually required here.

Happy to discuss.

vjkoskela · 2017-01-31T22:18:56Z

...n/java/com/arpnetworking/clusteraggregator/configuration/ClusterAggregatorConfiguration.java

@@ -429,6 +455,8 @@ public Builder setDatabaseConfigurations(final Map<String, DatabaseConfiguration
        private RebalanceConfiguration _rebalanceConfiguration;
        @NotNull
        private String _clusterHostSuffix = "";
+        @NotNull
+        private Props _clusterJoinActor = new NonJoiningClusterJoiner.Builder().build();


This isn't actually an actor. Do we want it to be? In the past we've done that through Guice, but as this is configuration I am inclined to say it's Props, but the name should probably reflect that.

At the end of the day it becomes an actor. I don't want the people configuring the service to have to know that they're actually building a Props that will get created into an actor. As far as they know, they want to configure an actor.

Also, the class that you give it in the config is an actor. It's just the actor builder that need to build a Props to create the actor.

BrandonArp · 2017-02-01T08:36:05Z

I've addressed most of the concerns. The implementation doesn't feel quite right still, but the semantics of the config file match what I want. It feels like there is still something to do for the declared type on the config classes, or perhaps in the way it polymorphically deserialize (or, rather, the way it doesn't use the built-in Jackson polymorphic deserialization).

BrandonArp force-pushed the actor_builder branch from efd0198 to b364e8a Compare December 20, 2016 08:39

BrandonArp force-pushed the actor_builder branch 2 times, most recently from 84b691f to 8b0bb52 Compare December 29, 2016 07:41

BrandonArp force-pushed the actor_builder branch 3 times, most recently from 0788e13 to bbcee86 Compare January 9, 2017 18:32

BrandonArp force-pushed the actor_builder branch from bbcee86 to 952af70 Compare January 9, 2017 19:05

BrandonArp force-pushed the actor_builder branch from 952af70 to fd6921c Compare January 19, 2017 08:29

BrandonArp force-pushed the actor_builder branch from fd6921c to 4afb2db Compare January 29, 2017 08:13

vjkoskela requested changes Jan 31, 2017

View reviewed changes

BrandonArp force-pushed the actor_builder branch 2 times, most recently from 4bb2285 to ac364ff Compare January 31, 2017 07:16

vjkoskela reviewed Jan 31, 2017

View reviewed changes

BrandonArp force-pushed the actor_builder branch from b73765f to 18a22d9 Compare February 1, 2017 08:31

BrandonArp force-pushed the actor_builder branch 2 times, most recently from 1837c08 to 3cc4086 Compare February 1, 2017 08:55

BrandonArp force-pushed the actor_builder branch 2 times, most recently from e8a63f4 to b420e27 Compare February 22, 2017 09:19

BrandonArp added 2 commits March 15, 2017 16:48

create actor builder, add cluster join actor

5a50c8f

PR feedback, move the actor builder closer to desired

290c558

BrandonArp force-pushed the actor_builder branch from b420e27 to 290c558 Compare March 15, 2017 23:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Actor builder and updates #51

Actor builder and updates #51

BrandonArp commented Dec 19, 2016

BrandonArp commented Dec 30, 2016

BrandonArp commented Jan 9, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017

vjkoskela Jan 31, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017

vjkoskela Jan 31, 2017

vjkoskela Jan 31, 2017

BrandonArp Jan 31, 2017 •

edited

Loading

BrandonArp commented Feb 1, 2017

Actor builder and updates #51

Are you sure you want to change the base?

Actor builder and updates #51

Conversation

BrandonArp commented Dec 19, 2016

BrandonArp commented Dec 30, 2016

BrandonArp commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrandonArp Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

BrandonArp commented Feb 1, 2017

BrandonArp Jan 31, 2017 •

edited

Loading