-
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
Schema changes for tags and using Akka Serialization for payloads #467
Conversation
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.
looking good
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.
@chbatey, I did a first pass. I will do a more detailed one later.
Left a few comments here and there.
I think we should rebase this with #465 (if I get it green). Some important pieces have moved.
We should think how we want to test the old DAO, maybe we should have one job (Scala 2.13, JDK11 and old DAO).
One more thing, some headers are missing, this will show up in the build. Need to run a sbt headersCreateAll
.
@@ -259,7 +264,7 @@ jdbc-snapshot-store { | |||
# to the same value for these other journals. | |||
use-shared-db = null | |||
|
|||
dao = "akka.persistence.jdbc.snapshot.dao.ByteArraySnapshotDao" | |||
dao = "akka.persistence.jdbc.snapshot.dao.AkkaSerializerSnapshotDao" |
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.
I think we can call it DefaultSnapshotDao
and DefaultReadJournalDao
and we don't need to expose the impl detail that it's using Akka serialization.
This is also meant to be the default DAO, so ok to have a short name.
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.
may I insist on changing the namesto DefaultSnapshotDao
and DefaultReadJournalDao
? 😉
core/src/main/scala/akka/persistence/jdbc/config/AkkaPersistenceConfig.scala
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/jdbc/journal/dao/AkkaSerializerJournalDao.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/jdbc/journal/dao/AkkaSerializerJournalDao.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/jdbc/journal/dao/legacy/ByteArrayJournalDao.scala
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/jdbc/journal/dao/AkkaSerializerJournalDao.scala
Outdated
Show resolved
Hide resolved
8ce0f4c
to
2525ec8
Compare
2041ddd
to
2ae32b2
Compare
What is remaining here? Anything that needs additional review? |
Ready for a final review. Nothing specific to look at.
…On Mon, Jan 4, 2021 at 8:43 AM Patrik Nordwall ***@***.***> wrote:
What is remaining here? Anything that needs additional review?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#467 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOHYG5IXZSCQVWWB2TCPELSYF5SBANCNFSM4UWM4DWA>
.
|
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.
Looking really good.
I think we are almost there. I left a few comments and suggestions here and there.
Some things needs to be tackled before merging tough.
@@ -259,7 +264,7 @@ jdbc-snapshot-store { | |||
# to the same value for these other journals. | |||
use-shared-db = null | |||
|
|||
dao = "akka.persistence.jdbc.snapshot.dao.ByteArraySnapshotDao" | |||
dao = "akka.persistence.jdbc.snapshot.dao.AkkaSerializerSnapshotDao" |
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.
may I insist on changing the namesto DefaultSnapshotDao
and DefaultReadJournalDao
? 😉
core/src/main/resources/schema/mysql/mysql-create-schema-v5.sql
Outdated
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/jdbc/journal/dao/JournalQueries.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/jdbc/testkit/internal/SchemaType.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/akka/persistence/jdbc/testkit/internal/SchemaUtilsImpl.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/akka/persistence/jdbc/query/JournalDaoStreamMessagesMemoryTest.scala
Show resolved
Hide resolved
core/src/test/scala/akka/persistence/jdbc/query/JournalSequenceActorTest.scala
Show resolved
Hide resolved
If you must but i think it is inconsistent with the name of the other impl which does include how it serialises in the name |
core/src/main/scala/akka/persistence/jdbc/testkit/internal/SchemaUtilsImpl.scala
Outdated
Show resolved
Hide resolved
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.
LGTM!
Raising so can keep track of progress. Plenty still to do