alberto-art3ch commented on PR #5951:
URL: https://github.com/apache/fineract/pull/5951#issuecomment-4827740810
> @alberto-art3ch Please consider the below recommendations:
>
> ### PR #5951 (Savings COB) — does it match Loan COB?
> Short answer: **no, not in the way you want.** The Spring Batch _step
topology_ is a faithful port, but the PR does **not**reuse the generic COB
framework that Loan COB already sits on — it re-introduces monolithic,
copy‑pasted implementations under `Savings*` names. On top of that, several
pieces are missing or wrong enough that the job can't compile, start, or
actually do anything.
>
> #### The core problem: it duplicates instead of reusing the existing
generic layer
> Loan COB was already refactored into entity‑agnostic bases in the
**`fineract-cob`** module, and the Loan classes are thin (30–60 line)
subclasses. The PR ignored all of them and re-implemented the pre‑refactor
monoliths. Verified mappings:
>
> PR's new Savings class | Existing generic base it should extend (in
fineract-cob) | What Loan does -- | -- | -- SavingsCOBPartitioner (~107 lines)
| cob.common.CommonPartitioner | LoanCOBPartitioner extends CommonPartitioner
(58 lines) SavingsLockingServiceImpl (~117 lines) |
cob.domain.AbstractLockingService | LoanLockingServiceImpl extends
AbstractLockingService (57 lines) ApplySavingsLockTasklet (~108 lines) |
cob.tasklet.ApplyCommonLockTasklet | ApplyLoanLockTasklet extends
ApplyCommonLockTasklet (47 lines) UnlockProcessedSavingsTasklet |
cob.tasklet.UnlockProcessedAccountsTasklet+ AccountLockService |
UnlockProcessedLoansTasklet extends UnlockProcessedAccountsTasklet<>
ChunkProcessingSavingsItemListener(~103 lines) |
cob.listener.AbstractLoanItemListener(already generic) |
ChunkProcessingLoanItemListener extends AbstractLoanItemListener (39 lines)
SavingsItemReader | cob.service.BeforeStepLockingItemReaderHelper |
LoanItemReader delegates to it (44 lines) new Sav
ingsLockOwner enum | existing cob.domain.LockOwner | reuses LockOwner
> The uniform approach you're after already exists. Savings COB should
shrink to ~6 thin subclasses + supply entity-specific
table/SQL/lock-owner/parameter keys, exactly like Loan and Working‑Capital‑Loan
do. Where a base is "Loan"‑named but already generic
(`AbstractLoanItemListener<T>`, `AbstractLoanItemReader<T>`), rename it to a
neutral `Abstract*`/`Common*` as part of this PR.
>
> #### Verified findings, most severe first
> 1. **`UnlockProcessedSavingsTasklet:44` won't compile** — calls
`savingsAccountLockRepository.removeOrphanedLocksForProcessedAccounts()`, but
that method does not exist on `SavingsAccountLockRepository` (it only has
`removeLockByOwner()` / `existsBy…`). The generic
`AccountLockService.removeOrphanedLocksForProcessedAccounts()` that Loan uses
_does_ exist — reuse it. (Note `removeLockByOwner` deletes rows where `error IS
NOT NULL` — the opposite of orphan cleanup.)
> 2. **Lock table is never created** — `SavingsAccountLock` maps to
`@Table(name = "m_savings_account_locks")`, but no Liquibase changelog creates
that table (`grep` finds none; Loan ships `0039_add_loan_account_locks.xml`).
First `applyLock`→ SQL error on every tenant.
> 3. **`batchJdbcTransactionTemplate` bean is undefined** — injected with
`@Qualifier("batchJdbcTransactionTemplate")` in
`SavingsCOBWorkerConfiguration`, `ApplySavingsLockTasklet:54`,
`ChunkProcessingSavingsItemListener`, but no such bean exists anywhere. Loan
uses the existing `requiresNewTransactionJdbcTemplate`
(`JdbcTransactionConfig:51`, `PROPAGATION_REQUIRES_NEW`). →
`NoSuchBeanDefinitionException` at startup.
> 4. **Retry contract broken in `ApplySavingsLockTasklet`** — gates retries
on `contribution.getStepExecution().getCommitCount()`(diff line 729) instead of
the persisted attempt counter `ApplyCommonLockTasklet` uses
(`executionContext.getLong(getApplyLockAttemptsKey())`, lines 88–99).
`getCommitCount()` is committed-chunk count, not failed-lock attempts, so `> 3`
is typically never true → tasklet returns `CONTINUABLE` and can spin instead of
failing after 3 attempts. Disappears entirely if you extend the base.
> 5. **Stayed-locked detection is a no-op** —
`RetrieveSavingsIdServiceImpl.findAllStayedLockedByCobBusinessDate` returns
`List.of()` with comment _"implemented when we add the query… lock table
doesn't exist yet"_. So `StayedLockedSavingsTasklet` can never emit
`SavingsAccountsStayedLockedBusinessEvent`.
> 6. **Job has no business steps → stops immediately** — there is zero
`implements SavingsCOBBusinessStep` and no `m_batch_business_steps` seed for
`SAVINGS_CLOSE_OF_BUSINESS`. `SavingsCOBPartitioner` calls `stopJobExecution()`
when steps are empty, so every run stops without processing anything.
> 7. **No `job` row → never scheduled** — no migration inserts a "Savings
COB" `ScheduledJobDetail` (Loan ships `0037_add_loan_cob_job_data.xml`). The
bean exists but the scheduler never triggers it and it's invisible in the
Scheduler Jobs API.
> 8. **`PartitionedJob` enum not extended** — still only `LOAN_COB`. A stuck
`SAVINGS_COB` execution fails `existsByJobName`, so
`StuckJobExecutorServiceImpl` treats it as a plain tasklet job — stuck-job
recovery diverges from Loan.
>
> Also worth fixing:
`ApplySavingsLockTasklet`/`ChunkProcessingSavingsItemListener` mutate a shared
`TransactionTemplate` via `setPropagationBehavior(...)` per call (diff 450,
766) under the multi-threaded worker step — a data race the Loan path avoids by
using a pre-configured bean; and `AbstractSavingsItemReader` throws raw
`RuntimeException` instead of a typed not-found exception (loses meaning in the
recorded lock stacktrace).
>
> #### Recommendation
> Reframe the PR around the existing `fineract-cob` generics rather than
parallel copies: extend `CommonPartitioner`, `AbstractLockingService`,
`ApplyCommonLockTasklet`, `UnlockProcessedAccountsTasklet<T>`,
`AbstractLoanItemListener<T>` (rename it), `BeforeStepLockingItemReaderHelper`,
and reuse `LockOwner`/`AccountLockService`. Then add the missing artifacts Loan
ships as standard — the lock-table migration, the `job` row, the business-step
seed, at least one `SavingsCOBBusinessStep` impl, and the `PartitionedJob` enum
entry. That gets you the uniform, single-maintenance-point COB you're
describing, and every future COB fix applies to both account types at once.
Thanks @adamsaghy for the detailed review. We are applying the next updates
**Reuse over duplication** (the main point): Savings COB now sits on the
generic fineract-cob framework instead of re-implementing the pre-refactor
monoliths. _SavingsCOBPartitioner_, _SavingsLockingServiceImpl_,
_ApplySavingsLockTasklet_, _ChunkProcessingSavingsItemListener_ and the readers
are now thin subclasses of _CommonPartitioner_, _AbstractLockingService_,
_ApplyCommonLockTasklet_, _AbstractItemListener_ (renamed from
_AbstractLoanItemListener_) and _AbstractAccountItemReader_ (extracted from
_AbstractLoanItemReader_). _SavingsLockOwner_/_SavingsLockingService_ were
dropped in favor of the shared _LockOwner_/_LockingService_. Net ~-110 LOC and
a single maintenance point for both account types.
**Bugs fixed** for free by the reuse: the retry contract (now uses the
persisted ExecutionContext attempt counter, not getCommitCount()), the
shared-TransactionTemplate data race (pre-configured
requiresNewTransactionJdbcTemplate), the typed not-found exception in the
reader, and PartitionedJob.SAVINGS_COB so stuck-job recovery matches Loan.
**One deliberate exception**: _UnlockProcessedSavingsTasklet_ still uses the
repository directly rather than _UnlockProcessedAccountsTasklet<T>_ — folding
_SavingsAccountLock_ into the loan_id-keyed _AccountLock_ hierarchy is a
larger, riskier entity refactor I'd prefer to do separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
