-
Notifications
You must be signed in to change notification settings - Fork 3
VED-859 Improve ack backend efficiency and make completion process more robust #892
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
Merged
dlzhry2nhs
merged 9 commits into
master
from
feature/VED-859-improve-ack-backend-process
Oct 10, 2025
Merged
VED-859 Improve ack backend efficiency and make completion process more robust #892
dlzhry2nhs
merged 9 commits into
master
from
feature/VED-859-improve-ack-backend-process
Oct 10, 2025
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket: VED-859 |
47f6a81 to
9812230
Compare
9812230 to
2606e31
Compare
mfjarvis
reviewed
Oct 10, 2025
mfjarvis
reviewed
Oct 10, 2025
JamesW1-NHS
reviewed
Oct 10, 2025
|
mfjarvis
approved these changes
Oct 10, 2025
dlzhry2nhs
added a commit
that referenced
this pull request
Oct 13, 2025
dlzhry2nhs
added a commit
that referenced
this pull request
Oct 14, 2025
mfjarvis
pushed a commit
that referenced
this pull request
Oct 22, 2025
mfjarvis
pushed a commit
that referenced
this pull request
Oct 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.



Summary
Problem is summarised in full on ticket: https://nhsd-jira.digital.nhs.uk/browse/VED-859
Aims to deal with 2 problems:
Solution
Set number of records processed from ECS in DynamoDB - this is a reliable full record count using CSV Dict Reader.
Use the row id - format is {batch_message_id}^{record_number} in the ack lambda events to check if we have reached the final record by comparing to the value saved to DynamoDB
This means we no longer read the full S3 source file on every invocation and perform the expensive row count check.
There is still some slowness, as record ordering is critical and we cannot have parallel writers to the same S3 object, so we need to append each time to the ack file.
More details on ticket, but on a 50MB file we saw a 33% decrease in overall time taken to process. This time saving may increase with larger files.
Further options considered
Before settling on the above option, I did think about reading the source file (properly with CSV reader to avoid the newlines issue) once and caching the value, but realised this would be susceptible to the same encoding issue we have to handle in ECS. Not to mention ECS already counts the total records.
Additional changes:
Reviews Required
Review Checklist
ℹ️ This section is to be filled in by the reviewer.