Hi First of all thank you for your contribution.
Unfortunately I am pretty swallowed at the moment, so the review goes much slower than i anticipated. Thank you for your understanding. In the meantime, could you please take a look at the failing test cases? Regards, Adam S > On Sep 10, 2026, at 7:01 AM, Farooq Ayoade <[email protected]> wrote: > > Hi Adam, all, > > Thanks — I've gone with the third option. PR #6078 now rebuilds > EXECUTE_STANDING_INSTRUCTIONS as a remote-partitioned Spring Batch job on the > same machinery LOAN_COB already uses, rather than hand-rolling the chunking, > retry and parallelism. That let the four concerns from the review collapse > into > one change instead of a ladder of them: > > - A manager step partitions the due set; workers take a partition each and run > a chunk-oriented step. Sizes come from > fineract.partitioned-job.partitioned-job-properties, alongside LOAN_COB's. > > - Partitions are cut over distinct SOURCE ACCOUNTS rather than over > instructions, so every instruction debiting an account stays in one > partition and runs sequentially there. Two partitions therefore never > contend on the same savings account row. The partition key collapses the > savings and loan id spaces with COALESCE; those spaces overlap, so two > unrelated accounts can share a partition, which costs a little parallelism > and nothing else. The direction that matters holds: an account is never > split across partitions. > > - The reader pages through its partition by keyset over (priority, id) rather > than by offset. Executing an instruction stamps its last_run_date and so > removes it from the due set, so an offset page would step over as many > instructions as the previous page had just committed. > > - Chunk-then-per-item is Spring Batch's own fault-tolerant replay rather than > anything I wrote: a chunk is attempted in one transaction, and if any > instruction in it fails the chunk rolls back and the step replays it one > instruction per transaction. That replay is also what now delivers the > original bug fix — a failing instruction cannot leave a sibling reverted, > because the sibling is re-executed and committed on the replay. > > - Retry is Spring Batch's, scoped to transient failures only. An account short > of funds will not have more of them a moment later, so retrying only delays > the run and pads the mandate's history; it is skipped and recorded instead, > in its own transaction so the record outlives the rollback of the transfer. > No new dependency was needed, so Retry4j did not come into it. > > One thing the replay forced that is worth calling out, since it would bite any > job converted this way: an instruction can now be presented for execution > twice > within a run. Execution therefore claims the instruction for the business date > first, by conditionally stamping last_run_date, and transfers nothing if the > claim finds it already run. Without that, a rolled-back chunk could pay an > instruction that had already paid. > > On your point about insufficient funds being common for standing instructions, > and so the per-item fallback firing often: I kept chunk-first as you > suggested, > but the chunk size is configurable, so a deployment where that trade goes the > wrong way can set EXECUTE_STANDING_INSTRUCTIONS_CHUNK_SIZE=1 and get > per-instruction behaviour with no code change. If you would rather the shipped > default were 1, I am happy to change it — I have no field data on the real > failure rate, and I would rather not guess in a default. > > Two things I have deliberately left out of the PR: > > - The persisted next_run_date. It is a schema change, and question 1 from my > original mail — how it should react to backdated and valid_from edits — > never got an answer, so I would rather it had its own ticket and its own > discussion than ride along here. > > - Grouping partitions by the destination account as well as the source. That > is a union-find over the transfer graph, and the credit side does not carry > the balance check that makes the debit side contend, so the residual > contention there is what the retry is for. Happy to be told that is too > optimistic if anyone has seen otherwise in production. > > Two smaller fixes came along with it: the retrieval query ordered by priority > DESC while the enum is URGENT(1)..LOW(4), so the job worked through the lowest > priorities first; and the history row was written by a string-concatenated > INSERT recording the attempted amount narrowed to a double, rather than what > actually moved. > > Review very welcome: https://github.com/apache/fineract/pull/6078 > > Kind Regards, > Farooq > > > > > > On Friday, July 24, 2026 at 10:29:17 AM GMT+1, Farooq Ayoade > <[email protected]> wrote: > > > > > > Hi all, > Following up from Adam's review on PR #6078 — moving the design discussion > here as suggested.PR #6078 (FINERACT-2672) isolates standing-instruction > execution so a single failing instruction can no longer mark the whole run's > transaction rollback-only and revert successful transfers. It does this by > executing each instruction in its ownREQUIRES_NEW transaction and recording > failures in separate committed transactions.- JIRA: > https://issues.apache.org/jira/browse/FINERACT-2672- PR: > https://github.com/apache/fineract/pull/6078On the review, Adam raised a > broader set of scalability improvements. I'd suggest the isolation fix in > #6078 can land on its own correctness merits, with the scalability work > tracked as a separate ticket and designed here — but I'm happy to keepthem > together if folks prefer.Current state: the EXECUTE_STANDING_INSTRUCTIONS job > is a single Spring Batch tasklet that loads all due instructions into memory > in one SELECT, then loops. Due-ness is computed per row at runtime from > last_run_date + recurrence; there is nopersisted next-run date.Redesign > direction (roughly in dependency order):- Persisted next_run_date on the > instruction, so the fetch becomes an indexed WHERE next_run_date > <=:businessDate. This is the enabler for everything below.- Paginated/keyset > fetch in chunks (~100) instead of loading the full due set.- Chunk-oriented, > fault-tolerant step: attempt a chunk in one transaction; on failure, fall > back to per-item transactions (Spring Batch skip/scan semantics). Open > question: for SIs, insufficient-funds failures are common, so the per-item > fallbackmay fire frequently — worth validating the happy-path saving isn't > eaten by fallback re-execution.- Partitioned parallelism grouped by account > (from/to) so same-account instructions run sequentially within a partition, > avoiding lock contention/deadlocks.- Scoped retry (Retry4j or Spring Batch > retry): only for transient failures (deadlock, lock timeout, > service-unavailable) — explicitly not for business failures like insufficient > balance or validation errors.- Longer term: whether to move to Spring Batch > remote-partitioning/worker steps for the parallelization and retry rather > than hand-rolling it.Questions for the list:1. Is a persisted next_run_date > an acceptable schema addition, and how should it interact with backdated / > valid_from changes?2. For parallelism, is account-based partitioning > sufficient, or are there SI relationships (e.g., chained transfers) that need > stricter ordering?3. Appetite for the Spring Batch remote-worker model vs. a > simpler local partitioned step as the first iteration? > Happy to write this up as a design doc/wiki page if that's the preferred > format. > > > > Kind Regards, > Farooq > >
