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

New 'import' job to trigger return log and supplementary flag checks #1453

Open
wants to merge 47 commits into
base: main
Choose a base branch
from

Conversation

jonathangoulding
Copy link
Collaborator

https://eaflood.atlassian.net/browse/WATER-4728

This change adds a new 'import' job into the repo to trigger the work previously done to bring the legacy import logic.

This change adds a new endpoint to be triggered from jenkins and will use the existing process licence service.

There are a lot of licence refs to iterate over so a batching mechanism is used to reduce the load on the system.

https://eaflood.atlassian.net/browse/WATER-4728

This change adds a new 'import' job into the repo to trigger the work previously done to bring the legacy import logic.

This change adds a new endpoint to be triggered from jenkins and will use the existing process licence service.

There are a lot of licence refs to iterate over so a batching mechanism is used to reduce the load on the system.
jonathangoulding added a commit that referenced this pull request Oct 29, 2024
https://eaflood.atlassian.net/browse/WATER-4728

As part of ongoing work to migrate the legacy import logic into this repo, we are adding a trigger to enable us to start using this new logic - #1453

However, some of the logic needs to be turned off behind a feature flag so the other logic can go live.

This import process was initially designed to be kicked off by the existing import service. This change will remove the route and controller for this older idea.
@jonathangoulding jonathangoulding self-assigned this Oct 30, 2024
@jonathangoulding jonathangoulding added the enhancement New feature or request label Oct 30, 2024
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

There is some general cleanup around comments and ordering here.

The key suggestion I have though is about changing what work the 2 services do. Any questions or challenges on this do give me a ping.

app/services/jobs/import/process-import-licence.service.js Outdated Show resolved Hide resolved
app/services/jobs/import/process-import-licence.service.js Outdated Show resolved Hide resolved
app/services/jobs/import/import-licence.service.js Outdated Show resolved Hide resolved
app/controllers/jobs.controller.js Outdated Show resolved Hide resolved
app/controllers/jobs.controller.js Outdated Show resolved Hide resolved
@jonathangoulding
Copy link
Collaborator Author

Refactor added to move the process import licences into the import licences service.

@jonathangoulding jonathangoulding marked this pull request as draft November 19, 2024 13:38
add performance test for importing licecnes
@jonathangoulding jonathangoulding force-pushed the feature-import-job-to-trigger branch from bb48ad5 to 7a85f7b Compare November 20, 2024 09:04
@jonathangoulding jonathangoulding marked this pull request as ready for review November 20, 2024 09:05
add performance test for importing licecnes
add performance test for importing licecnes
Cruikshanks added a commit that referenced this pull request Dec 3, 2024
TBH, I'm still not clear when and how this service will get called. But the two scenarios I think we are looking at is

- when a change happens with a licence
- when [New 'import' job to trigger return log and supplementary flag checks](#1453) calls the service

If it errors in the first scenario, we want it to blow up and the user to see it so they are not led to believe that the return logs have been generated. And if we can process 5K return logs in 3 secs, we should be able to handle a single licence in 1 sec, so I feel there is no need for the time to be logged.

In the second instance, we'll be processing all 74K licences. We are only interested in those that have changed (end date added, removed or changed) but we normally see quite a number.

In that case like the return logs job, we're more interested in the time for processing _all_ changed licences, not individual ones. And unlike the return logs job, it is acceptable that a return log creation failing blows up _this_ process, because we are just handling a single licence. It will be on the import job to ensure a licence blowing up doesn't prevent other licences from be processed.
Cruikshanks added a commit that referenced this pull request Dec 6, 2024
TBH, I'm still not clear when and how this service will get called. But the two scenarios I think we are looking at is

- when a change happens with a licence
- when [New 'import' job to trigger return log and supplementary flag checks](#1453) calls the service

If it errors in the first scenario, we want it to blow up and the user to see it so they are not led to believe that the return logs have been generated. And if we can process 5K return logs in 3 secs, we should be able to handle a single licence in 1 sec, so I feel there is no need for the time to be logged.

In the second instance, we'll be processing all 74K licences. We are only interested in those that have changed (end date added, removed or changed) but we normally see quite a number.

In that case like the return logs job, we're more interested in the time for processing _all_ changed licences, not individual ones. And unlike the return logs job, it is acceptable that a return log creation failing blows up _this_ process, because we are just handling a single licence. It will be on the import job to ensure a licence blowing up doesn't prevent other licences from be processed.
@jonathangoulding jonathangoulding added the do not merge Used for spikes and experiments label Dec 11, 2024
ProcessReturnLogsService.go(cycle)

if (h.request.payload !== null && h.request.payload.licenceReference) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this might be a merge issue ?

i am not in the git blame @robertparkinson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Used for spikes and experiments enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants