Support unique constraint exceptions when running Vitess. #3386
Conversation
| transacter.transaction { session -> | ||
| session.save(DbMovie("Cinderella", LocalDate.of(1950, 3, 4))) | ||
| } | ||
| assertFailsWith<ConstraintViolationException> { | ||
| transacter.transaction { session -> | ||
| session.save(DbMovie("Beauty and the Beast", LocalDate.of(1991, 11, 22))) | ||
| session.save(DbMovie("Cinderella", LocalDate.of(2015, 3, 13))) | ||
| repeat(5) { // Repeat 5 times to trigger Vitess constraint on a single shard |
There was a problem hiding this comment.
Should something in the stack be doing these retries automatically?
There was a problem hiding this comment.
This was just to ensure we get at least two writes on a single shard. The unique constraint is limited to each shard.
There was a problem hiding this comment.
NVM, a single insert causes the constraint violation. Not sure why I needed additional inserts before.
| use(previous) | ||
| try { | ||
| use(previous) | ||
| } catch (th: GenericJDBCException) { |
There was a problem hiding this comment.
| } catch (th: GenericJDBCException) { | |
| } catch (exception: GenericJDBCException) { |
| use(previous) | ||
| } catch (th: GenericJDBCException) { | ||
| // Ignore the exception if the connection is already closed. | ||
| if (!isConnectionClosed(th)) { |
There was a problem hiding this comment.
| if (!isConnectionClosed(th)) { | |
| if (!isConnectionClosed(exception)) { |
| } catch (th: GenericJDBCException) { | ||
| // Ignore the exception if the connection is already closed. | ||
| if (!isConnectionClosed(th)) { | ||
| throw th |
There was a problem hiding this comment.
| throw th | |
| throw exception |
| } | ||
| } else { | ||
| function() | ||
| } | ||
| } | ||
|
|
||
| private fun isConnectionClosed(th: GenericJDBCException) = |
There was a problem hiding this comment.
| private fun isConnectionClosed(th: GenericJDBCException) = | |
| private fun isConnectionClosed(exception: GenericJDBCException) = |
| } | ||
| } else { | ||
| function() | ||
| } | ||
| } | ||
|
|
||
| private fun isConnectionClosed(th: GenericJDBCException) = | ||
| th.cause?.javaClass == SQLException::class.java && |
There was a problem hiding this comment.
| th.cause?.javaClass == SQLException::class.java && | |
| exception.cause?.javaClass == SQLException::class.java && |
| } | ||
| } else { | ||
| function() | ||
| } | ||
| } | ||
|
|
||
| private fun isConnectionClosed(th: GenericJDBCException) = | ||
| th.cause?.javaClass == SQLException::class.java && | ||
| th.cause?.message.equals("Connection is closed") |
There was a problem hiding this comment.
| th.cause?.message.equals("Connection is closed") | |
| exception.cause?.message.equals("Connection is closed") |
| @@ -368,9 +367,6 @@ abstract class TransacterTest { | |||
|
|
|||
| @Test | |||
| fun constraintViolationCausesTransactionToRollback() { | |||
| // Uniqueness constraints aren't reliably enforced on Vitess | |||
| assumeTrue(!transacter.config().type.isVitess) | |||
There was a problem hiding this comment.
If this test is run on non-Vitess databases, should it stay and within an if block that stops it from running for Vitess?
There was a problem hiding this comment.
I'm not sure I follow. This block was skipping this test when we're running Vitess. We want to run this test for Vitess as well.
|
Is this PR still active / wanting to be merged? Or has it been superceded by another one? |
Description
misk-hibernate didn't properly handle
ConstraintViolationExceptions when running with Vitess.After the retries are complete, the
ConstraintViolationExceptionshould be thrown, but in the case of Vitess, asession.target(previous)throws aGenericJDBCExceptionbecause the connection is already closed.This PR catches the
GenericJDBCExceptionand ignores it.Testing Strategy
Update the existing test to include Vitess.
Thank you for contributing to Misk! 🎉