-
Notifications
You must be signed in to change notification settings - Fork 142
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
WIP: Added support for serializing using Akka serialization #199
Conversation
Added support for serializing using Akka serialization including migration
- Fixed a mistake in the configuration example for custom database providers - Improved documentation on thread/connection pool sizing (specifically by removing the vague 5 * number of cores recommendation and pointing to a better resource)
Thanks @WellingR. Good idea to open a serialization branch for this. |
case _ if serializer.includeManifest => | ||
payload.getClass.getName | ||
case _ => "" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be replaced by
val serManifest = akka.serialization.Serializers.manifestFor(serializer, payload)
I'm testing this branch and it looks to me Oracle migration does not work - probably this is something to do with distinguishing empty and NULL blobs. Will try to find out why it happens. |
It looks to me slick has a bug here: https://github.com/slick/slick/blob/master/slick/src/main/scala/slick/jdbc/OracleProfile.scala#L155 Slick generates something like this for migration code (I shortened) and on non-migrated db it should return ALL rows, but no result returned: By changing from |
- scala 2.12.6 - akka 2.5.16 - scalatest 3.0.5 (test only) - postgres 42.2.5 (test only) - mysql 8.0.12 (test only)
Another bug to add here: |
Adds SQL Server schema and test updates. I created a derived SQL server [image](https://hub.docker.com/r/topaztechnology/mssql-server-linux/) that allows login and database creation on startup to facilitate testing. Write performance was initially poor vs Postgres, but significantly improved with use of `transactionally` in `BaseByteArrayJournalDao.writeJournalRows` This was based on a closed PR #108, thanks to @naferx
…efinitions. Rows per transaction has been made configurable.
It looks like the things left to do here are related to migrations. What is the status of this feature for new projects without old events to support? |
@renatocaval @skisel @jroper Do we still want to make these improvements even given the fact that migrations are needed? I would be happy to contribute myself, but time is an issue. |
@WellingR, we have talked about this with the Akka team recently. We will need to make time to move one with that. We don't have it yet planned, but it's in our radar. Not only that but that tagging issue. Actually, tagging is the most important one. And because we need some migration tooling, it will be a good opportunity to tackle that one altogether. |
Also closing this one as we will be taking a different approach as discussed in #318 |
This is a continuation of #180
Todos (help wanted):
supportsSerialization
compatibility flag (the tests break if this is enabled)RowsPerTransaction
configuration value which is currently hardcoded. It might be an idea to includerowsPerTransaction
as a method parameter when the migration is started.