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

WIP: Added support for serializing using Akka serialization #199

Closed
wants to merge 14 commits into from

Conversation

WellingR
Copy link
Member

@WellingR WellingR commented Sep 2, 2018

This is a continuation of #180

Todos (help wanted):

  • Improve migration script see here and here (in progress: Introduce better logging. Fix for Oracle. Performance fix. #201)
  • Investigate if we can split out the legacy and new table definitions see here (in progress Investigation if the legacy and new table definitions can be split out #203)
  • Make sure that the tests use both the new and the legacy configurations.
  • Re-enable the supportsSerialization compatibility flag (the tests break if this is enabled)
  • The journal migration tests should work for all supported types of databases. (Perhaps we could read the data from a csv file and convert that to some slick insert statements or something).
  • The journal migration uses a RowsPerTransaction configuration value which is currently hardcoded. It might be an idea to include rowsPerTransaction as a method parameter when the migration is started.
  • The changes from Add support for SQL Server #200 have been merged to master, these changes have been merged to this branch too, however there is no schema migration script yet. We should also investigate if additional tests are needed.
  • Fix oracle sql schema migration bug see here

jroper and others added 4 commits September 2, 2018 18:20
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)
@octonato
Copy link
Member

octonato commented Sep 3, 2018

Thanks @WellingR. Good idea to open a serialization branch for this.

case _ if serializer.includeManifest =>
payload.getClass.getName
case _ => ""
}
Copy link
Member

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)

@skisel
Copy link
Contributor

skisel commented Sep 8, 2018

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.

@skisel
Copy link
Contributor

skisel commented Sep 8, 2018

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:
select * from "journal" where dbms_lob.compare("event", NULL) = 0)
This actually works:
select * from "journal" where "event" is null

By changing from .filter(_.event.isEmpty) to .filter(_.serId.isEmpty) it actually works. I will try to improve migration and will submit PR.

@skisel
Copy link
Contributor

skisel commented Sep 8, 2018

#201

skisel and others added 3 commits September 15, 2018 15:28
- 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)
@skisel
Copy link
Contributor

skisel commented Sep 18, 2018

Another bug to add here:
sql> ALTER TABLE "journal" MODIFY "message" BLOB DEFAULT NULL [2018-09-18 23:08:08] [99999][22859] ORA-22859: invalid modification of columns

jonfox and others added 6 commits September 19, 2018 16:41
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.
@PerWiklander
Copy link

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?

@WellingR
Copy link
Member Author

WellingR commented Oct 1, 2019

@renatocaval @skisel @jroper
This PR has been silent for almost a year, it is now behind master and there are probably some conflicts that may need to be resolved.

Do we still want to make these improvements even given the fact that migrations are needed?
Is any contributor willing to spend the time to work on finishing the remaining work?

I would be happy to contribute myself, but time is an issue.

@octonato
Copy link
Member

octonato commented Oct 2, 2019

@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.

@octonato
Copy link
Member

octonato commented Mar 6, 2020

Also closing this one as we will be taking a different approach as discussed in #318

@octonato octonato closed this Mar 6, 2020
@octonato octonato removed this from the 4.0.0 milestone Mar 6, 2020
@ennru ennru deleted the akka-serialization branch November 19, 2020 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants