removing actorErrorStatesAgent in CubeSupervisor

[fmasion]

Hi,

There's already an issue about removing deprecated agent #471
But the as the solution might vary in each case I start this issue to have a discussion thread.

The actorErrorStatesAgent seem to be a patch to a broader design problem in this case :

The CubeStateBean accesses the actors internal state from an other thread (the PlatformMBeanServer) this should not be done
Some could argue that it's not a big deal because it's read only
that would at least mean the usage of @volatiles (that wouldn't make much sense in the context of an actor)
most properties never change so the missing @volatile isn't a big deal here because the thread-local cached value is the same and won't change...
The only property that could change is wrapped in an agent to make all this some kind of "thread safe again"

In this particular case we could replace the Agent[Map] by a ConcurrentHashMap for exemple but this doesn't address the original design problem : an actor's internal state accessed concurrently

Moreover the CubeSupervisor 's supervisorStrategy mutating the actor state to provide datas to the JMXBean feels to me like the business logic is polluted by cross concerns (monitoring and logging) we could achieve better separation of concerns here I think.

I like the way things are done in akka.cluster.ClusterJmx.scala :
https://github.com/akka/akka/blob/master/akka-cluster/src/main/scala/akka/cluster/ClusterReadView.scala

I would be happy to discuss about all this with you in this thread
Thanks

[akara]

Essentially, you're suggesting to publish state changes to the event bus and listen to the event bus and expose the state through a view object. Since these state changes are low frequency, this method works, too.

However, I'd like to note in the ClusterReadView example given here, the actor accesses and mutates the _state reference outside the scope of that actor. The result is similar. The state can be accessed synchronously, via a JMX MBean or otherwise.

But, by merit, having an MBean that reads the actor's internal state to make it visible or having an actor reach beyond its scope to modify the state outside of its thread of control is equally not quite right.

[akara]

The benefit of this model is you can have multiple listeners decoupled from the source of the event. The potential drawback if the state change is the frequent is that we'll have frequent events and object creations. For JMX, that value read is only happening when you actually access the MBean to read. Here we'd send a new object onto the event bus each state change.

As I said, the evil parts are either poking into an actor's state or let the actor make changes outside its scope. Neither of that is clean.

[fmasion]

Well in the ClusterReadView case the actor is only a listener performing side effets : mutating data available in read only from an other thread.
This make the usage on the "read side" using @volatile legitimate

in each case we will consume on one thread data produced on an other so we will always be stuck using some concurrent primitives.
it might be
-concurrent data structures like ConcurrentHashMap
-synchronisation or volatiles
-or make the JMX bean an actor

here's an other interesting article I found
https://github.com/typesafehub/activator-akka-jmx-example

[akara]

That's true, unless you care little about the data accuracy, or it is very slow-moving data as you mentioned in some of our use cases. So as long as the solve is also performance-optimized, I'm actually good with either solution - install a MXBean reading actor state from a different thread, or publish state change events. We just need to weigh the cost of creating an event for the particular use case.

Also, whether we really need concurrency primitives or we can trust the write to be atomic and not care much about thread memory cache, and therefore accuracy of the data is, IMO, also based on the data and change characteristics.

For instance, I would assume writing a long or swapping a pointer to be atomic operations.