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

[CPDLP-3521] Split contract migration into two migrators #1819

Closed
wants to merge 4 commits into from

Conversation

leandroalemao
Copy link
Contributor

@leandroalemao leandroalemao commented Oct 16, 2024

Context

Ticket: https://dfedigital.atlassian.net/browse/CPDLP-3521

We're seeing ERROR: cached plan must not change result type when migrating some contracts over.

Changes proposed in this pull request

It's a concurrency issue, where a contract template is being created then a linked contract is being initialised multiple times as we have multiple workers trying to create the same contract. We're splitting contract migrator into two new migrators (contract template and contract). Also limiting the workers # for the contract migrator.

image

Copy link

@leandroalemao leandroalemao force-pushed the fix-contracts-migrator branch 3 times, most recently from a9a2f5a to 73c664b Compare October 16, 2024 15:58
@leandroalemao leandroalemao changed the title [WIP][CPDLP-3521] Limit workers count for contracts migration [CPDLP-3521] Limit workers count for contracts migration Oct 16, 2024
Copy link
Collaborator

@cwrw cwrw left a comment

Choose a reason for hiding this comment

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

I think we shouldn't be picking up the same records through multiple workers because of this: https://github.com/DFE-Digital/npq-registration/blob/main/app/services/migration/migrators/base.rb#L60
However the interesting thing is here: https://github.com/DFE-Digital/npq-registration/blob/main/app/services/migration/migrators/base.rb#L54 this might be adding the extra records you mentioned as it's doing a ceiling on both, maybe we can discuss this. Let's chat tomorrow with @ethax-ross. Nice catch, thanks @leandroalemao.

Nvm I just reran and it raised the same failure with 1 worker, let's discuss tomorrow

@@ -30,6 +32,18 @@ def ecf_contracts
def dependencies
%i[course statement]
end

def number_of_workers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we just wrap the contract creation in a transaction to avoid the race conditions? alternatively I think instead of overriding the constant you could just do something like this I think?:

def number_of_workers
  1
end

def records_per_worker
  record_count
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ethax-ross tried that yesterday.. but it breaks some shared specs..

@leandroalemao leandroalemao force-pushed the fix-contracts-migrator branch 2 times, most recently from 13c4255 to 644379b Compare October 22, 2024 12:06
@leandroalemao leandroalemao changed the title [CPDLP-3521] Limit workers count for contracts migration [CPDLP-3521] Split contract migration into two migrators Oct 22, 2024
Copy link

sonarcloud bot commented Oct 23, 2024

@leandroalemao
Copy link
Contributor Author

Closing this for now. More info on https://dfedigital.atlassian.net/browse/CPDLP-3521.

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.

4 participants