Process billing batch move db writes #230
Merged
Conversation
This file contains 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
This PR aims to make `ProcessBillingBatchService` more performant by moving database writes out of the main loop.
In order to perform all db writing outside of the main processing loop, we want to change how we structure the stored billing data. The first part of this is refactoring our functions to avoid mutating data
Instead of having a `currentBillingData` object which we update as we iterate over the charge versions, we have a `billingData` object which is a keyed equivalent where the key is the billing invoice licence id and the value is effectively the `currentBillingData`. We update this as we iterate over the charge versions, then iterate over it after the loop to perform the db writing. Although this doesn't yet reduce the number of db writes, it does move them outside of the loop. As a bonus, we can simply update the desired billing invoice licence's data on each iteration and we no longer need to check for a change in id
The next step of separating out our db writes so we can do them in as few writes as possible is to refactor `_finaliseCurrentInvoiceLicence()` so that instead of reading the db to get previous transactions to cleanse then writing the results to the db, we have a separate `_generateCleansedTransactions()` function which does the reading and cleansing, leaving `_finaliseCurrentInvoiceLicence()` to do the writing
Instead of persisting data one item at a time as we iterate over `billingData`, we instead store all the transactions, invoices and invoice licences in arrays which we can then batch insert
When we loop over the billing data, we previously set the `isEmpty` flag to `false` before we add transactions, billing invoices and billing invoice licences to our `dataToPersist` object. Instead of setting the flag we can instead calculate it based on whether `dataToPersist.billingInvoiceLicences` is populated. We also rename it to `billingBatchIsEmpty` to make its intent a little clearer
On reflection, having a variable in our `go()` function to track whether the billing batch is empty isn't something we need. Instead, we refactor to pass the billing invoice licences through to `_finaliseBillingBatch()` and check in that function whether or not the array is empty
Previously we went through everything in the main loop, then checked to see if we should generate calculate transactions based on whether the charge version status is `current`. Since this last bit is the only part of the loop that does anything, we move the status check to the top of the loop so we can skip the iteration entirely if the status isn't `current`
Instead of blindly overwriting `billingData[billingInvoiceLicenceId]` with either the data we provided or some empty initial data, we check first to see if it exists and only write our defaults if it doesn't. This makes it clearer what we're doing (ie. "only set the initial data if something doesn't already exist")
We move the main loop into its own function and refactor it to use `.reduce()`
We move the billing data handling loop out of `go()` and into its own function
StuAA78
added
the
housekeeping
Cruikshanks
approved these changes
May 17, 2023
There was a problem hiding this comment.
In my opinion I think we could pass around more specific values than the bulk data objects in some places. I also think with all that is going on in this service there are a few things we could do elsewhere.
But I also know we need to move some stuff out ASAP to support the multi-year work. And with that done what remains I think screams out for splitting into smaller, more focused services.
So, I'm keeping those details to myself and approving this! We can catch up after on the final refactoring/cleanup 😁
Other than that, this is going to really help 💪
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.
https://eaflood.atlassian.net/browse/WATER-3996
This PR aims to make
ProcessBillingBatchServicemore performant by moving database writes out of the main loop. As part of this we undertake additional refactoring to clean up the main loop.