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

Lock liquibase between validate and update #29008

Merged
merged 1 commit into from Nov 7, 2022

Conversation

joggeli34
Copy link
Contributor

Lock liquibase for validate and update as suggested by:
liquibase/liquibase#2816 (comment)

Closes #25335

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Nov 3, 2022

Thanks for the contribution.

Please format the code change according to the message above.

Copy link
Member

@famod famod left a comment

Choose a reason for hiding this comment

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

I'm a bit puzzled by what is going to happen if this is run in conjunction with the upstream PR that resets some caches.
Do we have to roll back this change when that liquibase version lands?

liquibase.validate();
var lockService = LockServiceFactory.getInstance().getLockService(liquibase.getDatabase());
try {
lockService.waitForLock();
Copy link
Member

Choose a reason for hiding this comment

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

Is releaseLock() "lenient" in that it does not throw an exception if the lock has not been acquired?
What I have in mind is:

  • waitForLock() fails with an exception
  • finally kicks in
  • releaseLock() fails (assumption!) because the lock has not been acquired
  • the exception from finally shadows the one from waitForLock(), confusing the user

I'd move waitForLock() out of the try block or add a catch + addSuppressed() to the finally block.

This might seem like overengineering, but I had similar cases more often than not.

var lockService = LockServiceFactory.getInstance().getLockService(liquibase.getDatabase());
try {
lockService.waitForLock();
if (config.validateOnMigrate) {
Copy link
Member

Choose a reason for hiding this comment

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

So we're adding the overhead(?) of locking even in the case where we don't validate.

But given validate-on-migrate ist true by default it should be alright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The overhead should be minimal, as the first thing liquibase.update() does is to acquire the lock. But if it already has it, it will result more or less in a map lookup, this should not generate much overhead.

@joggeli34
Copy link
Contributor Author

The code must not be rollbacked after the upstream fix, as with this change it will prevent to load the changelog twice, once for validate and once for update

Copy link
Member

@famod famod left a comment

Choose a reason for hiding this comment

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

Ok, so that upstream change (the reset) will not kick in with explicit locking because it'll bail out earlier because it detects that the db is already locked (by the locking approach of this PR).

Let's see what CI has to say.

@famod famod added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 4, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Nov 4, 2022

Failing Jobs - Building 40bbf8c

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

@gsmet gsmet merged commit 7e5ccc0 into quarkusio:main Nov 7, 2022
@quarkus-bot quarkus-bot bot added this to the 2.15 - main milestone Nov 7, 2022
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Nov 7, 2022
@gsmet
Copy link
Member

gsmet commented Nov 7, 2022

Thanks!

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.

Liquibase lock not working correctly anymore
4 participants