-
Notifications
You must be signed in to change notification settings - Fork 2
feat: reconcilliation service #1294
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
base: main
Are you sure you want to change the base?
Conversation
will-larkin-nhs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XML docs on all functions pls
.../src/Functions/ReconciliationService/ReconciliationService.Function/ReconciliationService.cs
Outdated
Show resolved
Hide resolved
.../ReconciliationService/ReconciliationService.Core/Extensions/InboundMetricClientExtension.cs
Outdated
Show resolved
Hide resolved
application/CohortManager/src/Functions/CaasIntegration/receiveCaasFile/Dockerfile
Show resolved
Hide resolved
| COPY ./ReconciliationService /dotnet-function-app | ||
| WORKDIR /dotnet-function-app/ReconciliationService.Function/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| COPY ./ReconciliationService /dotnet-function-app | |
| WORKDIR /dotnet-function-app/ReconciliationService.Function/ | |
| COPY ./ReconciliationService/ReconciliationService.Functiom /src/dotnet-function-app | |
| WORKDIR /src/dotnet-function-app |
this matches the pattern for the rest of the dockerfiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work as the code in core doesn't get copied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok fine can you still add /src before /dotnet-function-app so it matches the rest of the project
.../Functions/ReconciliationService/ReconciliationService.Function/ParticipantReconciliation.cs
Show resolved
Hide resolved
application/CohortManager/src/Functions/Shared/Common/Interfaces/IStateStore.cs
Show resolved
Hide resolved
| if (!result) | ||
| { | ||
| _logger.LogWarning("Metric failed to add to the database, Message will be deferred"); | ||
| await messageActions.DeferMessageAsync(message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we deferring messages here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we cannot add it to the database deferring enables us to check and retry the message. ensuring its eventually written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defering messages can be a little tricky as you need to check for messages that have been deferred and it can lead to strange bugs where messages get missed. they also don't get retried automatically we need to go get them and then get their sequence number to retry them. if they are needing to be retried why not set set retry options on the service bus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I understand this will autoretry and i should just allow the exception to be thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say yes. I would consider setting the max retry count that can be done in code but also in the azure portal
when creating the ServiceBusClient you can set something like this:
var clientOptions = new ServiceBusClientOptions
{
RetryOptions = new ServiceBusRetryOptions
{
Mode = ServiceBusRetryMode.Exponential,
Delay = TimeSpan.FromSeconds(2),
MaxDelay = TimeSpan.FromSeconds(30),
MaxRetries = 5
}
};
await using var client = new ServiceBusClient(connectionString, clientOptions);this explains the use of ServiceBusRetryMode: https://azuresdkdocs.z19.web.core.windows.net/dotnet/Azure.Messaging.ServiceBus/7.19.0/api/Azure.Messaging.ServiceBus/Azure.Messaging.ServiceBus.ServiceBusRetryMode.html
more info on ServiceBusClientOptions: https://learn.microsoft.com/en-us/javascript/api/@azure/service-bus/servicebusclientoptions?view=azure-node-latest
.../src/Functions/ReconciliationService/ReconciliationService.Function/ReconciliationService.cs
Show resolved
Hide resolved
.../Functions/ReconciliationService/ReconciliationService.Function/ParticipantReconciliation.cs
Outdated
Show resolved
Hide resolved
.../Functions/ReconciliationService/ReconciliationService.Function/ParticipantReconciliation.cs
Outdated
Show resolved
Hide resolved
.../src/Functions/ReconciliationService/ReconciliationService.Function/ReconciliationService.cs
Show resolved
Hide resolved
application/CohortManager/src/Functions/Shared/Common/BlobStateStore.cs
Outdated
Show resolved
Hide resolved
application/CohortManager/src/Functions/Shared/Common/BlobStateStore/BlobStateStore.cs
Show resolved
Hide resolved
tests/UnitTests/ReconciliationServiceTests/ParticipantReconciliationTests.cs
Show resolved
Hide resolved
| blob_service_client.create_container("file-exceptions") | ||
| blob_service_client.create_container("nems-updates") | ||
| blob_service_client.create_container("nems-config") | ||
| blob_service_client.create_container("config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we name this something more descriptive/ specific?
| environment: | ||
| - ASPNETCORE_URLS=http://*:7162 | ||
| - AzureWebJobsStorage=${AZURITE_CONNECTION_STRING} | ||
| - ReconciliationTimer=5/5 * * * * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll need to enclose this value in quotes since there are spaces
| COPY ./ReconciliationService /dotnet-function-app | ||
| WORKDIR /dotnet-function-app/ReconciliationService.Function/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok fine can you still add /src before /dotnet-function-app so it matches the rest of the project
|



Description
Adding a new function which will provide 3 main methods
This change adds the functionality to ensure that all participants received from the file and API are accounted for in either the exception table or Cohort Distribution.
Note the implementation of the Reconcile participant class is liable to change as the application evolves and is in a state which is tightly coupled to the current implementation.
Context
DTOSS-9195
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.