Migration guide from v2 to v3

[KarelCemus]

Hi,

this pull request intends to fix #60.

1. I added changelog into README documenting the stashing behavior was dropped
2. I modified the RedisConnectionSupervisor to notify newly registered listeners about current state to ensure their proper initialization based on current connection state + fixed a test
3. I implemented StashingRedis actor. It is designed as a proxy in front of the Redis actor. It listens to its state changes and based on them it passes incoming requests through or stashes them. It's also documented. I added 2 new tests.

May I ask you for the feedback? Is acceptable this way or any modification are needed?

[damienlevin]

Thanks for your contribution, the change looks good ! Few comments :

• Can you update the StashingRedis actor with a configurable bounded mailbox ? This will guaranty that the application doesn't continually stash messages. The memory footprint of stashing thousands of requests can be really significant.
• Can you update the version and the README accordingly ?
• The StashingRedis actor can't be use with a sharded connection, can you mention that in the README ?

[KarelCemus]

Sure.

Can you update the StashingRedis actor with a configurable bounded mailbox ?

What should be default mailbox size?

Can you update the version and the README accordingly ?

What should be the new version? 3.0.3-SNAPSHOT?

Thanks

[KarelCemus]

@damienlevin There is a patch:

1) implemented LeakingStash dropping oldest messages when it is full 2) default capacity set to 50 messages but I'll change it if you tell me preferred value 3) documented in README that it cannot be used with the sharded connection 4) bumped version to 3.0.3-SNAPSHOT

[KarelCemus]

@damienlevin Is there anything else to fix in this PR?

Thanks

[chrisdinn]

LGTM.

@damienlevin any objection to me merging this?

[damienlevin]

Sorry I've been busy lately, I'll take a look over the weekend !

[chrisdinn]

Okay, let's target a merge on Monday morning (NYC time).

[jasongoodwin]

Just a few thoughts here: Instead of defining the bounding in the Leaky Stashing Actor with code, we could use a bounded deque mailbox for the actor. This means that bounded vs unbounded becomes a configuration choice instead of requiring any code. We can define mailboxes in configuration and then create the actor withMailbox as needed. Capacity then is defined in the configuration for the bounded mailbox.

Actors with stash need to use the deque mailbox so for a bounded mailbox we can use the bounded deque mailbox and define the capacity there:

bounded-stash-mailbox {
  mailbox-type = "akka.dispatch.BoundedDequeBasedMailbox"
  mailbox-capacity = 1000
  mailbox-push-timeout-time = 10s
}

Creating an actor with a bounded mailbox is then as simple as using withMailbox method on Props. We could consider having props factory methods for the two different choices for example.

context.actorOf(Props[RedisStashActor].withMailbox("bounded-stash-mailbox"))

Or the actor can have the requirement mixed in to the actor if we always want the bounded mailbox type on the redis actor

LeakyRedisStashActor extends RedisStashActor with RequiresMessageQueue[BoundedDequeBasedMessageQueueSemantics]. 

[damienlevin]

+1 with Jason, I would much prefer that approach.

[KarelCemus]

I am not sure I understand what exactly are you trying to refactor out. The capacity? Or something else?

The thing is, there is no (or at least I have not found any) leaky mailbox. Using a mailbox as you suggested would eventually end up by throwing and exception when the capacity is reached. In conclusion, it requires writing a custom implementation. Of course, we can provide a custom leaking mailbox implementation but it seems to me like much of work with nearly no benefit.

Or is there something I am missing?

[jasongoodwin]

Hey I'll demonstrate and reply later. The behavior of the bounded mailbox should be equivalent (eg to drop messages) so we should be able to get the same behavior without maintaining any code. I'll just confirm and shoot back an example when I get home. The intention would be to use Akka's mechanics instead of rolling custom code when it already does have the same semantics and behavior. On Nov 23, 2015 5:40 PM, "Karel Čemus" notifications@github.com wrote:

I am not sure I understand what exactly are you trying to refactor out. The capacity? Or something else?

The thing is, there is no (or at least I have not found any) leaky mailbox. Using a mailbox as you suggested would eventually end up by throwing and exception when the capacity is reached. In conclusion, it requires writing a custom implementation. Of course, we can provide a custom leaking mailbox implementation but it seems to me like much of work with nearly no benefit.

Or is there something I am missing?

— Reply to this email directly or view it on GitHub https://github.com/chrisdinn/brando/pull/61#issuecomment-159090706.

[jasongoodwin]

Hey, So I had a chance to put together a quick example that shows Akka can handle the behaviour with config. Actually for stash you just have to set the "stash-capacity" on the mailbox. Here is a working test case demonstrating the behaviour (eg it will take the first 50 messages and drop the rest gracefully.) It logs exceptions but I think that's not a bad thing - you could shut that up if you want by adjusting logging but it seems like a sane default behaviour.

https://github.com/jasongoodwin/akka-bounded-stash-example

This might not be a bad way to do it - if I can help in any way please let me know!

I guess also some watch-outs are that eg stash has some caveats around duplicate messages getting stashed I think. Haven't run into that myself but I did notice that in the docs.

[KarelCemus]

Thanks for the quick response but you've made my point.

eg it will take the first 50 messages and drop the rest gracefully. It logs exceptions

This is what I do not want. Let's assume the exceptions are fine. However, I dislike the first 50 messages and drop the rest part. My implementation, which I think is meaningful here, takes LAST 50 messages and drops the oldest. And that is something the Akka does not implement or at least I haven't found this implementation.

Motivation: I assume the redis is a cache and when we cannot provide a data the app computes them. So dropping the messages is not that biggy. Similarly, I think we should preserve the newest messages as the oldest might timeout.

And this is how my implementation works and I don't think we can do it with current Akka utils.

However, I admit my intended behavior does not have to fit your intentions.

Thoughts?

[jasongoodwin]

Ya I can see where you're coming from with redis being a k/v store - I was thinking we were just trying to avoid the leak causing the app to crash.

It's an edge case and I'm fine either way. I'm thinking you can still process things out of order if you drop the oldest messages: if you want at least once delivery then you'll timeout and retry anything you don't get an ack on. If you drop older messages, then you would be getting timeouts on older messages, while the newest messages are handled successfully. So you'd timeout and retry the older messages, reprocessing them after the newest messages. If you drop the newest messages you'll timeout and retry them so your odds of avoiding shooting yourself in the foot might be a bit better. It all ends up falling back on the client application in the end.

[KarelCemus]

It's an edge case You are right, I didn't realize that. Then I think it's fine to have it either way.

Than I'll accept @jasongoodwin suggestions and do it the configuration way without leaking. I'll also work in the comments in the code.

@jasongoodwin @damienlevin @chrisdinn When I do these changes, would it be acceptable as a PR or is there anything else I need to do? I'd like to finish this. Thanks

[damienlevin]

Perfect, I'll merge that when it's fixed

[KarelCemus]

@damienlevin Please check my changes. I removed the LeakyStash and implemented plain akka.Stash instead, which can be restricted in a configuration as @jasongoodwin showed.

[damienlevin]

The change looks great, I'll merge that in tonight !

[KarelCemus]

Thanks!