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 deploymentId not populating after locking #4450

Merged
merged 2 commits into from Jun 29, 2023
Merged

Conversation

wwillard7800
Copy link
Contributor

@wwillard7800 wwillard7800 commented Jun 28, 2023

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

Fix issue: #4448
I spent a while trying to implement a FastCheckCommandStep class because the root cause really is we want to not lock the database if fastcheck reports we are up to date, but ending up stumbling on a number of issues relating to the different usages of the LockServiceCommandStep. Because the lock service step is so "low level" it doesn't really work to add the necessary parameters to support a fast check. I thought about implementing a FastCheckLockServiceCommandStep but just resolved I was getting in too deep and should be able to fix this in a few lines of code for now.

I'm not sure if FastCheckLockServiceCommandStep is the right move for the future or if we should choose some other route.

Things to be aware of

Things to worry about

Additional Context

Copy link
Collaborator

@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.

Thanks! That's not the first time that this happens.. if I'm right before 4.21.0 we had a similar issue.

@abrackx abrackx changed the title Test to validate that DEPLOYMENT_ID is populated on update Fix deploymentId not populating after locking Jun 28, 2023
@abrackx abrackx self-assigned this Jun 28, 2023
@abrackx abrackx removed their request for review June 28, 2023 19:00
@MalloD12 MalloD12 linked an issue Jun 28, 2023 that may be closed by this pull request
@MalloD12
Copy link
Contributor

MalloD12 commented Jun 28, 2023

Hey @wwillard7800 / @abrackx , I'm seeing there is another call of:

Scope.getCurrentScope().getSingleton(ChangeLogHistoryServiceFactory.class).getChangeLogService(database).generateDeploymentId();

in AbstractFutureRollbackCommandStep. Should that one also be removed or is not used? I don't have all this command process super clear, but thought it might be worth checking with you.

Thanks,
Daniel.

@wwillard7800
Copy link
Contributor Author

Hey @wwillard7800 / @abrackx , I'm seeing there is another call of:

Scope.getCurrentScope().getSingleton(ChangeLogHistoryServiceFactory.class).getChangeLogService(database).generateDeploymentId();

in AbstractFutureRollbackCommandStep. Should that one also be removed or is not used? I don't have all this command process super clear, but thought it might be worth checking with you.

Thanks, Daniel.

I believe that this call needs to be here. @abrackx do you agree?

@abrackx
Copy link
Contributor

abrackx commented Jun 29, 2023

Hey @wwillard7800, I'm seeing there is another call of:

Scope.getCurrentScope().getSingleton(ChangeLogHistoryServiceFactory.class).getChangeLogService(database).generateDeploymentId();

in AbstractFutureRollbackCommandStep. Should that one also be removed or is not used? I don't have all this command process super clear, but thought it might be worth checking with you.

Thanks, Daniel.

Good catch but I believe this is irrelevant to the bug. future-rollback-<count/tag/etc>-sql commands only yield rollback scripts and do not touch (nor operate on) deployment id. As far as I know, the only times deployment id really come into play is when running an update.

@filipelautert filipelautert merged commit a0210a2 into master Jun 29, 2023
28 of 29 checks passed
@filipelautert filipelautert deleted the gh-issue-4448 branch June 29, 2023 12:13
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.

DeploymentId not populated after waiting for lock
4 participants