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

Resets changelog cache upon acquiring lock #3396

Conversation

filipelautert
Copy link
Collaborator

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

A clear and concise description of the change being made.

Things to be aware of

  • None

Things to worry about

    • how does it affects performance?

Additional Context

Add any other context about the problem here.

@github-actions
Copy link

github-actions bot commented Oct 21, 2022

Unit Test Results

  4 668 files  ±  0    4 668 suites  ±0   31m 0s ⏱️ - 3m 13s
  4 642 tests +18    4 416 ✔️ +21     226 💤  - 3  0 ±0 
54 708 runs  +12  49 589 ✔️ +17  5 119 💤  - 5  0 ±0 

Results for commit 4f26ee4. ± Comparison against base commit 1fb44ca.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@nvoxland nvoxland left a comment

Choose a reason for hiding this comment

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

This is not the right spot for this resetAll.

The different history services should be managing their state correctly for normal usage, and the reset call should be more for one-off "we know we are doing something strange, and so need to tell them to reset" things.

Adding a rest to the update operation is working around whatever the underlying bug is. Perhaps too aggressive caching in the implementation? Perhaps a specific case where we should know that the implementations can't know what's changing under them and do the reset there?

@nvoxland nvoxland removed their assignment Oct 25, 2022
filipelautert and others added 4 commits October 28, 2022 16:54
… we get the lock we are going to have a clean cache.
…rrectly-due-to-cache-of-ranchangesetlist' into 2816-upgrade-lock-not-working-correctly-due-to-cache-of-ranchangesetlist
Copy link
Collaborator Author

@filipelautert filipelautert left a comment

Choose a reason for hiding this comment

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

Tks, changed as discussed.

@filipelautert filipelautert changed the title Resets changelog cache before running update Resets changelog cache upon acquiring lock Oct 31, 2022
Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

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

PR handles cases where multiple threads are attempting to run the same changesets due to a stale cache of ranChangeSetList. This situation was encountered by users running multiple simultaneous Liquibase Docker instances. Fix resets the ranChangeSetList to prevent the contents from becoming stale over the lifetime of the lock.

APPROVED

@XDelphiGrl
Copy link
Contributor

@filipelautert and @FBurguer, hello!

We should attempt to recreate the bug and then validate the fix is working. The linked issue gives a code snippet that can be used to trigger the situation. The user initially encountered the problem running with multiple Liquibase Docker containers. I believe that either the code snippet or using multiple Docker containers are equally good choices for testing.

I suggest timeboxing the reproduction of the bug. Threading issues are difficult to recreate and can cost more time than is reasonable to merge the fix to master to see if the PR addresses the issue for the broader community.

-erz

@filipelautert
Copy link
Collaborator Author

@XDelphiGrl @FBurguer I'm attaching here a java class that can be used to test it.

I run 2 instances with the following parameters (it need ot be 2 processes, can't be threads that's why 2 executions):

  • t1 15000
  • t2 1

image

I hope it helps!

RunnableProblem.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Upgrade lock not working correctly due to cache of ranChangeSetList
5 participants