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

fix: Integration tests not working correctly #806

Merged
merged 3 commits into from Mar 12, 2024
Merged

Conversation

johanandren
Copy link
Member

No description provided.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.


// FIXME this test doesn't pass because of something with SequenceNextValUpdater
Copy link
Member

Choose a reason for hiding this comment

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

Issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I pinged the contributor in in the PR where he just added this the other day

@steventwheeler
Copy link
Contributor

@johanandren here's a patch which should resolve the failing tests.

fix-mysql-tests.PATCH

@johanandren
Copy link
Member Author

@steventwheeler I'm afraid not, I think the error is related to not getting any rows back from the MySQLSequenceNextValUpdater nextValFetcher query:

 Cause: java.util.NoSuchElementException: empty.head
[info]   at scala.collection.immutable.Vector.head(Vector.scala:288)
[info]   at akka.persistence.jdbc.state.scaladsl.JdbcDurableStateStore.$anonfun$insertDurableState$1(JdbcDurableStateStore.scala:223)

I'll merge this and we can follow up with a fix for the disabled/failing test.

@johanandren johanandren merged commit 94ec2a7 into master Mar 12, 2024
14 checks passed
@johanandren johanandren deleted the wip-ci-tests-fix branch March 12, 2024 15:52
@steventwheeler
Copy link
Contributor

@johanandren sorry about that it was an issue with my test setup. I was seeing different errors. I've been able to reproduce what you saw in the checks and am struggling a bit due to my inexperience with Scala & Slick. I've got a version that works with MariaDB, but it doesn't work with MySQL as MySQL hasn't implemented INSERT ... RETURNING .... Any chance you could point me in the right direction on how to implement a getSequenceNextValueExpr() that executes two queries in a transaction? I've tried this, but it fails due to no implicit ExecutionContext.

def getSequenceNextValueExpr() = {
  val insertAction = sqlu"""INSERT INTO durable_state_global_offset_sequence (id) VALUES (DEFAULT(id))"""
  val selectAction = sql"""SELECT LAST_INSERT_ID()""".as[String].head
  (for {
    insertRes <- insertAction
    selectRes <- selectAction
  } yield selectRes).transactionally
}

This relies on a table to generate unique ID values similar to the sequence used in H2:

CREATE TABLE IF NOT EXISTS durable_state_global_offset_sequence (
    id BIGINT NOT NULL AUTO_INCREMENT,
    PRIMARY KEY (id));

Alternatively, if you guys are okay with only supporting MariaDB this works just fine:

def getSequenceNextValueExpr() = sql"""INSERT INTO durable_state_global_offset_sequence (id) VALUES (DEFAULT(id)) RETURNING id""".as[String]

@johanandren
Copy link
Member Author

Hmm, MySQL is what we officially support, with MariaDB being more in a "likely working" status, so that is probably not good enough.

It's not entirely obvious to me if a two step operations expression can be squeezed into getSequenceNextValueExpr, I'd need to dig a bit deeper to understand that but I don't have that many cycles to spare right now so it may be a while.

@steventwheeler
Copy link
Contributor

Fair enough. At this point I think it makes more sense for me to write my own adapter rather than try and figure out how to make MySQL work with your interface. It's probably a good idea to revert my previous commit so someone doesn't try and use it.

@johanandren
Copy link
Member Author

Ok, reverting in #807 for now then

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.

None yet

3 participants