Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrading dynamodb journal plugin to Akka 2.4 #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pcleary00
Copy link

This project had a dependency on spray-aws, which had a dependency on
akka 2.3 and spray. Spray is no longer supported moving forward, so I
changed the dependency to only require the aws sdk.

There are some substantial changes in Akka 2.4 for persistence, most
notably:

  • WriteConfirmations go away, so there is no more concept of imperanent
    deletes. A lot of code around write confirmations was removed from
    the plugin
  • The asyncWriteMessages signature changed. Instead of taking a
    Seq[PersistentRepr], it instead takes a Seq[AtomicWrite]. There are
    also certain guarantees around the handling of AtomicWrites that must
    be implemented; namely that every AtomicWrite has an associated
    Try[Unit] that is returned. This means that every AtomicWrite should
    yield a successful result. I had to change the way we handled
    asyncWriteMessages to always ensure a response.

I beleive that this plugin will be backward compatible, as I did not
alter the way the items are being built.

I created an integration-test.sh that will run the tests. One of the
tests was pulled from Martin Krasser's journal plugin; the other test
uses the JournalSpec.

I did not due the JournalPerfSpec as I am only running locally, and I
was not able to get the throughput hitting the amazondb local.

@sclasen
Copy link
Owner

sclasen commented Oct 13, 2015

@pcleary00 looking good. One bit of history/information here, when this project was initially developed as a journal for https://github.com/eligosource/eventsourced, the initial implementation was closer to what you have where it used the amazon dynamodb client directly.

While this worked, in practice it had performance bottlenecks. It was actually the reason I first built https://github.com/sclasen/spray-aws. I saw much better throughput using it. (Testing with tables up to 30,000 Write units)

Ideally, spray-aws would get ported over to akka-http instead of spray, and this project could use it. I am bandwidth constrained atm so that wont happen by my hand any time soon however.

Maybe we can just take this as is, and do the spray-aws => akka-aws port at a later date if/when throughput becomes an issue?

Let me know if/when you get a chance to use this on non-local Dynamodb, and if you see any throughput issues. Is this something you could try relatively easily?

@pcleary00
Copy link
Author

I plan on using this against AWS this week. Once I get a test area I will
retry the perf test and let you know the results.

In the meantime I am upgrading my app to Akka 2.4 and this updated Dynamo
plugin. I will have it deployed to a staging area this week as well. We
have a good number of integration tests that will exercise the plugin in an
application setting.

If the performance seems acceptable I would propose we use this version for
the initial release. I can look at upgrading spray AWS after we get over
some of these short term hurdles this month.

Thanks!

On Tuesday, October 13, 2015, Scott Clasen [email protected] wrote:

@pcleary00 https://github.com/pcleary00 looking good. One bit of
history/information here, when this project was initially developed as a
journal for https://github.com/eligosource/eventsourced, the initial
implementation was closer to what you have where it used the amazon
dynamodb client directly.

While this worked, in practice it had performance bottlenecks. It was
actually the reason I first built https://github.com/sclasen/spray-aws. I
saw much better throughput using it. (Testing with tables up to 30,000
Write units)

Ideally, spray-aws would get ported over to akka-http instead of spray,
and this project could use it. I am bandwidth constrained atm so that wont
happen by my hand any time soon however.

Maybe we can just take this as is, and do the spray-aws => akka-aws port
at a later date if/when throughput becomes an issue?

Let me know if/when you get a chance to use this on non-local Dynamodb,
and if you see any throughput issues. Is this something you could try
relatively easily?


Reply to this email directly or view it on GitHub
#12 (comment)
.

This project had a dependency on spray-aws, which had a dependency on
akka 2.3 and spray.  Spray is no longer supported moving forward, so I
changed the dependency to only require the aws sdk.

There are some substantial changes in Akka 2.4 for persistence, most
notably:
- WriteConfirmations go away, so there is no more concept of imperanent
  deletes.  A lot of code around write confirmations was removed from
the plugin
- The asyncWriteMessages signature changed.  Instead of taking a
  Seq[PersistentRepr], it instead takes a Seq[AtomicWrite].  There are
also certain guarantees around the handling of AtomicWrites that must
be implemented; namely that every AtomicWrite has an associated
Try[Unit] that is returned.  This means that every AtomicWrite should
yield a successful result.  I had to change the way we handled
asyncWriteMessages to always ensure a response.

I beleive that this plugin will be backward compatible, as I did not
alter the way the items are being built.

I created an `integration-test.sh` that will run the tests.  One of the
tests was pulled from Martin Krasser's journal plugin; the other test
uses the JournalSpec.

I did not due the JournalPerfSpec as I am only running locally, and I
was not able to get the throughput hitting the amazondb local.
@pcleary00
Copy link
Author

I setup throughput of 1000/1000. Running from my laptop, I got the following results:

[info] + PersistAsync()-ing 10000 took 20516 ms
[info] + PersistAsync()-ing 10000 took 17774 ms
[info] + PersistAsync()-ing 10000 took 18162 ms
[info] + PersistAsync()-ing 10000 took 17344 ms
[info] + PersistAsync()-ing 10000 took 17261 ms
[info] + PersistAsync()-ing 10000 took 17430 ms
[info] + PersistAsync()-ing 10000 took 17422 ms
[info] + PersistAsync()-ing 10000 took 18038 ms
[info] + PersistAsync()-ing 10000 took 21223 ms
[info] + PersistAsync()-ing 10000 took 17060 ms
[info] + Average time: 18223 ms

Which I guess makes sense, kinda maxing out throughput if I read the numbers based on a 1000/1000 provisioned table, considering each one is actually 2 puts (I think I have that right, I am pretty sure the journal plugin does 2 writes per object).

I did get one of these, not sure if you ever saw it when running your tests...

eplaying event type [akka.persistence.journal.JournalPerfSpec$Cmd] with sequence number [4204] for persistenceId [p-16].
java.lang.IllegalArgumentException: requirement failed: Expected to receive [4198] yet got: [4199]
    at scala.Predef$.require(Predef.scala:219)
    at akka.persistence.journal.JournalPerfSpec$BenchActor$$anonfun$receiveRecover$1.applyOrElse(JournalPerfSpec.scala:54)
    at scala.runtime.AbstractPartialFunction.apply(AbstractPartialFunction.scala:36)
    at akka.persistence.Eventsourced$$anon$3$$anonfun$1.applyOrElse(Eventsourced.scala:438)
    at akka.actor.Actor$class.aroundReceive(Actor.scala:480)
    at akka.persistence.journal.JournalPerfSpec$BenchActor.akka$persistence$Eventsourced$$super$aroundReceive(JournalPerfSpec.scala:15)
    at akka.persistence.Eventsourced$$anon$4.stateReceive(Eventsourced.scala:481)
    at akka.persistence.Eventsourced$class.aroundReceive(Eventsourced.scala:158)
    at akka.persistence.journal.JournalPerfSpec$BenchActor.aroundReceive(JournalPerfSpec.scala:15)
    at akka.actor.ActorCell.receiveMessage(ActorCell.scala:525)
    at akka.actor.ActorCell.invoke(ActorCell.scala:494)
    at akka.dispatch.Mailbox.processMailbox(Mailbox.scala:257)
    at akka.dispatch.Mailbox.run(Mailbox.scala:224)
    at akka.dispatch.Mailbox.exec(Mailbox.scala:234)
    at scala.concurrent.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)
    at scala.concurrent.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
    at scala.concurrent.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)
    at scala.concurrent.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)```

@pcleary00
Copy link
Author

One more thing, I went through the actual table, and was looking at the entries around the exception up above. There were like 150,000 entries, so limited only to something that came close...

I kinda find it odd that those with a deleted of FALSE don't have a sequence number

Here are the entries, you can let me know if you see anything out of the ordinary...

key (String) deleted (String) sequenceNr (Number)
journal-SH-p-16-4995 4995
journal-P-p-16-4992 FALSE
journal-SH-p-16-4993 4993
journal-P-p-16-4999 FALSE
journal-P-p-16-499 FALSE
journal-P-p-16-4991 FALSE
journal-SH-p-16-4998 4998
journal-P-p-16-4997 FALSE
journal-P-p-16-4995 FALSE
journal-SH-p-16-4990 4990
journal-SH-p-16-499 499
journal-SH-p-16-4996 4996
journal-SH-p-16-4992 4992
journal-P-p-16-4990 FALSE
journal-SH-p-16-4994 4994
journal-SH-p-16-4997 4997
journal-P-p-16-4998 FALSE
journal-P-p-16-4996 FALSE
journal-SH-p-16-4999 4999
journal-P-p-16-4994 FALSE
journal-SH-p-16-4991 4991
journal-P-p-16-4993 FALSE

@pcleary00
Copy link
Author

I did more testing on this, and it is working great on recovery and operations in an application setting. I have this version deployed in an early release of my application. I will let you know if I hit any issues.

@pcleary00
Copy link
Author

@sclasen Any updates on this PR? Let me know thoughts on next steps, I should be getting freed up in the next few days

@analytically
Copy link

👍

@rkuhn
Copy link

rkuhn commented Jan 30, 2016

@pcleary00 Good news on this one: the Akka team is taking over maintainership of this plugin, the new home is at akka/akka-persistence-dynamodb. I am currently getting up to speed, yesterday I have set up Travis to build and test the plugin and the next step is to perform the upgrade to Akka 2.4.1. Would you be open to submitting this PR against the new repo? (you’ll have to sign version 2.2 of the Typesafe CLA, I can see that you signed 2.1 earlier)

And sorry that it took so long to answer this PR, @sclasen told me he was exceptionally busy off late.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants