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.

Sign up for GitHub

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

Jump to bottom

Add support for SQL Server #200

Merged
merged 1 commit into from
Sep 19, 2018
Merged

Add support for SQL Server #200

merged 1 commit into from
Sep 19, 2018

Conversation

jonfox
Copy link
Contributor

@jonfox jonfox commented Sep 5, 2018

Adds SQL Server schema and test updates. I created a derived SQL server image 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

@WellingR
Copy link
Member

WellingR commented Sep 6, 2018 •
edited
Loading

Awesome! Perhaps it is an idea to merge this after #199.

In that case we do not need support for the legacy schema for sql server.

I still need to have a thorough look at this. I also noticed one if the travis builds failed (just tried to restart it).

@jonfox
Copy link
Contributor Author

jonfox commented Sep 6, 2018

Yes I've noticed there seem to be intermittent failures on the 2.11 build. I have a Travis build hooked up to my fork and it passed maybe 50% of the time, so presumably load related. I haven't had a chance to investigate yet.

Also Codacy seems to be incorrectly complaining about valid T-SQL schema, not sure if it is possible to tag the SQL dialect.

In terms of merging once everything is reviewed and behaving, I have a fairly prompt requirement so would prefer sooner rather than later if at all possible, not sure what the timeline is for #199. I'd be happy to work on any updates required as part of that PR.

@WellingR
Copy link
Member

WellingR commented Sep 6, 2018 •
edited
Loading

It might take a while before #199 is completely ready. So perhaps we can merge this one before.

The unstable travis build is a blocker though. This needs some investigation. Perhaps it is possible to reduce the number of events in the specific tests. I have also done this for some of the oracle tests which had some stability issues in the past.

@WellingR WellingR added this to the 3.5.0 milestone Sep 7, 2018
@jonfox
Copy link
Contributor Author

jonfox commented Sep 10, 2018

Sure I will take a look at this and get it passing reliably on Travis

@jonfox
Copy link
Contributor Author

jonfox commented Sep 14, 2018

It looks like rebasing on top of your dependency-updates branch, and updating mssql-jdbc to 7.0.0 has Travis passing now.

@jonfox
Copy link
Contributor Author

jonfox commented Sep 17, 2018

or maybe not ... back to the drawing board.

@jonfox jonfox force-pushed the sqlserver-support branch 2 times, most recently from 5c8e0d6 to f875548 Compare September 18, 2018 13:59
@jonfox
Copy link
Contributor Author

jonfox commented Sep 18, 2018

I think the tests are passing reliably now, I've run Travis hooked up to my repo 10 times against the latest commit successfully.

The problem seemed to be down to the transactionally change; it looks like H2 doesn't like this when writing large batches of messages, so I have excluded use with the H2 profile. Of course it's preferable the write should be atomic, and SQL Server performance is very poor otherwise.

WellingR
WellingR approved these changes Sep 18, 2018
View reviewed changes
Copy link
Member

@WellingR WellingR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@WellingR WellingR mentioned this pull request Sep 18, 2018
Closed
8 tasks
octonato
octonato approved these changes Sep 19, 2018
View reviewed changes
Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot for this.

src/main/scala/akka/persistence/jdbc/journal/dao/ByteArrayJournalDao.scala
db.run(queries.writeJournalRows(xs).transactionally).map(_ => Unit)
else
// However transactionally causes H2 tests to fail
db.run(queries.writeJournalRows(xs)).map(_ => Unit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why H2 is failing when using transactionally? That's not normal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot I had added this comment and merge it.

In any case, I don't think we are doing any harm here. Actually, I run this locally without the if/else and calling transactionally. All H2 tests passed.

Moreover, we don't need it transactionally because we have only one DBIO. transactionally is only needed when we have many DBIOs and we want to run them all together in the same tx. That's not the case here. Here we have one DBIO with many row inserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the test failures were intermittent. Locally I also didn't have a problem, but maybe the Travis hardware was lower spec and caused the circuit breaker to time out. It's also not clear why this would reduce performance in H2 in the first place.

With regards to not requiring transactionally, it certainly made a significant difference to SQL Server performance. It's calling JournalTableC ++= xs.sortBy(_.sequenceNumber) but I have no idea how Slick handles this internally.

src/test/scala/akka/persistence/jdbc/journal/JdbcJournalSpec.scala
@@ -62,6 +62,7 @@ class PostgresJournalSpecSharedDb extends JdbcJournalSpec(ConfigFactory.load("po
class PostgresJournalSpecPhysicalDelete extends JdbcJournalSpec(ConfigFactory.load("postgres-application.conf")
.withValue("jdbc-journal.logicalDelete", ConfigValueFactory.fromAnyRef(false)), Postgres())

class MySQLJournalSpec extends JdbcJournalSpec(ConfigFactory.load("mysql-application.conf"), MySQL())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! That one was missing and nobody noticed it. Thanks

build.sh
@@ -17,7 +17,9 @@ done;
}
rm ./bintray.sbt

wait 5432 PostgreSQL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, PostgreSQL was not included by default?

@octonato octonato merged commit 816258e into akka:master Sep 19, 2018
@jonfox jonfox deleted the sqlserver-support branch October 16, 2018 12:30
@WellingR WellingR mentioned this pull request Feb 3, 2019
Closed
evgenyzic pushed a commit to evgenyzic/akka-persistence-jdbc that referenced this pull request Dec 16, 2019
@jonfox @evgenyzic
Add support for SQL Server (akka#200)
2c82c44 
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 akka#108, thanks to @naferx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@octonato octonato octonato approved these changes

@WellingR WellingR WellingR approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jonfox @WellingR @octonato