Uploaded image for project: 'mod-finance-storage'
  1. mod-finance-storage
  2. MODFISTO-260

Transaction summaries are not designed to be used in parallel





      When 2 users or separate mods run operations in parallel, they can send both summaries before sending the operations. mod-finance-storage is not saving previous summaries with the same id (which in practice is the order id for encumbrances), so one of the summaries sent will have no effect, resulting in an error for the second one:

      "All expected transactions already processed"


      Redesign transaction summaries to allow parallel usage. Using unique ids (as opposed to for instance order ids)  could be part of the solution, but we don't want to accumulate too many records in the summary tables, so they would have to be deleted when no longer in use.

       When redesigning, care should be taken to avoid requiring a check for 404 errors, as was done in mod-orders' TransactionSummariesService.java for MODORDERS-545 (normal operations should not report errors in the logs).

      Example of an issue

      The mod-orders integration test delete-opened-order-and-lines triggers the deletion of related encumbrances when the fund distribution is removed. There are 2 encumbrances to delete, and mod-orders starts the deletion of both in parallel (as would happen if 2 users were deleting the 2 transactions at the same time). It calls mod-finance, which releases the transactions before deletion. The transaction PUT requires the use of a summary, so mod-finance creates a summary before the update (for each transaction, in parallel).

      This is mod-finance's log:

      2021-08-25T11:37:37,333 INFO [vert.x-eventloop-thread-1] RestRouting invoking deleteFinanceEncumbrancesById
      2021-08-25T11:37:37,336 INFO [vert.x-eventloop-thread-1] RestRouting invoking deleteFinanceEncumbrancesById
      2021-08-25T11:37:37,363 INFO [vert.x-eventloop-thread-1] CommonTransactionService Start releasing transaction e5ef504e-2ff2-4166-ab2e-40b8a8d09166
      2021-08-25T11:37:45,478 INFO [vert.x-eventloop-thread-1] CommonTransactionService Start releasing transaction 6ecfb1ce-7a62-425c-9347-3a8ec4fe1b6e
      2021-08-25T11:37:48,133 ERROR [vert.x-eventloop-thread-1] TransactionsApi Exception encountered
      org.folio.rest.exception.HttpException: All expected transactions already processed

      An error occurs because mod-finance-storage is not expecting 2 summaries+transactions to happen at the same time for the same order or invoice.


      One could wonder if it is safe to allow 2 callers to edit transactions for the same order at the same time. If we decided to not allow it, mod-finance-storage could return an error when a summary is posted while the previous one for the same order/invoice has not been fully completed. It would require changing mod-orders somehow to deal with the example issue given above. A risk in that approach is that if a list of transactions planned with a summary never finishes, the transactions can never be updated again (unless we add some kind of timeout mechanism).

      The API could also be fixed to allow parallel processing for series of transaction creations and updates. Instead of using the order id or invoice id as a key for the list of edits, we could use the summary id. The caller would get the id as it is returned by the POST method for the summary, and use it for each edit afterwards. Since 2 callers would use 2 different ids, there would be no risk of collision. It would also simplify the API because the same calls could be done for orders and invoices. Internally, mod-finance-storage would need a way to save the summary id at the same time as a temporary transaction, perhaps by storing it simply in the transaction. Temporary transactions would be removed when executed, when the total from the summary is reached.

      With that new API, it would be safer to check for parallel edits and deny them if we choose to, because we could just search the temporary transactions for the same order/invoice id as a new one with a different summary id, and return an error if any is found.

      This new API would address issues such as the one in mod-orders' TransactionSummariesService, because a new id would always be used, and there would be no risk of conflict with an existing one. So we would no longer have errors in the logs for normal operations, and we would not have to check for 404 errors in calling modules.

      Changing the API would require changes in other modules. The old API should be preserved until all dependent modules have been updated.

      Summing up what needs to be done:
      1. Create a new model "TempTransaction" to support the correlation/summary "id" from the table "order_transaction_summaries".
       "TempTransaction" model should extends all field from "Transaction" with addition field: "summaryId". "TempTransaction" will be saved into "temporary_order_transactions"
      2. Table "order_transaction_summaries":
        2.1. Add new field "orderId" (old field "id" was "orderId", now "id" will be correlation id and "orderId" will be the order id)
       2.2. Existing field "id" will be unique and was generated on POST and should be set in the TempTransaction -> summaryId
       2.3 Update logic of the org.folio.service.transactions.AllOrNothingTransactionService#processAllOrNothing
      3. Transaction POST/PUT use a new transactionSummaryId query parameter, replacing orderId/invoiceId as the summary id. Remove TransactionSummaryService$getSummaryId().

      1. Update logic in all the places where we call transaction summary API to support updated "order_transaction_summaries" with field "orderId" and "id" should be generated and unique.
      The same for invoices
      Seems like it will be enough to support concurrent transactions update.

      1. Update API to pass the summary id for all transaction POST and PUT.

      TestRail: Results


          Issue Links



                siarhei_hrabko Siarhei Hrabko
                damieng Damien Guillaume
                0 Vote for this issue
                3 Start watching this issue



                  TestRail: Runs

                    TestRail: Cases