You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "ruchiD (via GitHub)" <gi...@apache.org> on 2023/06/14 10:09:05 UTC
[GitHub] [fineract] ruchiD opened a new pull request, #3255: FINERACT-1724-COB-hardlocked-loans-and-inline-cob-issue
ruchiD opened a new pull request, #3255:
URL: https://github.com/apache/fineract/pull/3255
## Description
Changes to prevent race condition for applying lock and executing cob between regular loan cob and Inline COB.
loan cob retry apply loan lock implementation.
tests.
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update unit or integration tests for verifying the changes made.
- [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy merged pull request #3255: FINERACT-1724-COB-hardlocked-loans-and-inline-cob-issue
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #3255:
URL: https://github.com/apache/fineract/pull/3255
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy commented on a diff in pull request #3255: FINERACT-1724-COB-hardlocked-loans-and-inline-cob-issue
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3255:
URL: https://github.com/apache/fineract/pull/3255#discussion_r1229609010
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/ApplyLoanLockTasklet.java:
##########
@@ -41,13 +43,43 @@
@RequiredArgsConstructor
public class ApplyLoanLockTasklet implements Tasklet {
+ private static final int NUMBER_OF_RETRIES = 3;
private final FineractProperties fineractProperties;
private final LoanLockingService loanLockingService;
private final RetrieveLoanIdService retrieveLoanIdService;
private final CustomJobParameterResolver customJobParameterResolver;
@Override
public RepeatStatus execute(@NotNull StepContribution contribution, @NotNull ChunkContext chunkContext) throws Exception {
+ // TODO: Need to check Spring Batch based retry logic in the future, with using RepeatStatus.CONTINUABLE to
+ // retry tasklet execution rather than do while. Currently with RepeatStatus.CONTINUABLE we are getting
+ // transaction rollback exception.
+ int numberOfExecutions = 0;
+ do {
+ try {
+ applyLoanLock(contribution);
+ return RepeatStatus.FINISHED;
+ } catch (Exception e) {
+ if (++numberOfExecutions > NUMBER_OF_RETRIES) {
+ String message = "There was an error applying lock to loan accounts.";
+ log.error("{}", message, e);
+ throw new LoanLockCannotBeAppliedException(message, e);
+ } else {
+ waitBeforeRetry();
+ }
+ }
+ } while (true);
+ }
+
+ private void waitBeforeRetry() {
+ try {
+ Thread.sleep(1000);
+ } catch (InterruptedException e) {
+ log.info("{}", "Error applying loan lock.", e);
Review Comment:
Should be rethrown the error here...and logging an error should be used the ERROR level, but at least the WARNING (but usually rather ERROR level)
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy commented on a diff in pull request #3255: FINERACT-1724-COB-hardlocked-loans-and-inline-cob-issue
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3255:
URL: https://github.com/apache/fineract/pull/3255#discussion_r1235337207
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/ApplyLoanLockTasklet.java:
##########
@@ -66,16 +76,36 @@ public RepeatStatus execute(@NotNull StepContribution contribution, @NotNull Chu
List<LoanAccountLock> accountLocks = new ArrayList<>();
loanIdPartitions.forEach(loanIdPartition -> accountLocks.addAll(loanLockingService.findAllByLoanIdIn(loanIdPartition)));
- List<Long> alreadyLockedByChunkProcessingAccountIds = accountLocks.stream()
- .filter(e -> LockOwner.LOAN_COB_CHUNK_PROCESSING.equals(e.getLockOwner())).map(LoanAccountLock::getLoanId).toList();
-
List<Long> toBeProcessedLoanIds = new ArrayList<>(loanIds);
- toBeProcessedLoanIds.removeAll(alreadyLockedByChunkProcessingAccountIds);
+ List<Long> alreadyLockedAccountIds = accountLocks.stream().map(LoanAccountLock::getLoanId).toList();
+
+ toBeProcessedLoanIds.removeAll(alreadyLockedAccountIds);
+ try {
+ applyLocks(toBeProcessedLoanIds);
+ } catch (Exception e) {
+ if (++numberOfExecutions > NUMBER_OF_RETRIES) {
Review Comment:
I think we dont need this. The commit count got increased with each execution.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy commented on a diff in pull request #3255: FINERACT-1724-COB-hardlocked-loans-and-inline-cob-issue
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3255:
URL: https://github.com/apache/fineract/pull/3255#discussion_r1229616961
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/ApplyLoanLockTasklet.java:
##########
@@ -41,13 +43,43 @@
@RequiredArgsConstructor
public class ApplyLoanLockTasklet implements Tasklet {
+ private static final int NUMBER_OF_RETRIES = 3;
private final FineractProperties fineractProperties;
private final LoanLockingService loanLockingService;
private final RetrieveLoanIdService retrieveLoanIdService;
private final CustomJobParameterResolver customJobParameterResolver;
@Override
public RepeatStatus execute(@NotNull StepContribution contribution, @NotNull ChunkContext chunkContext) throws Exception {
+ // TODO: Need to check Spring Batch based retry logic in the future, with using RepeatStatus.CONTINUABLE to
+ // retry tasklet execution rather than do while. Currently with RepeatStatus.CONTINUABLE we are getting
+ // transaction rollback exception.
+ int numberOfExecutions = 0;
+ do {
+ try {
+ applyLoanLock(contribution);
+ return RepeatStatus.FINISHED;
Review Comment:
I was wondering on this and i think we can try the following:
- start a new transaction for the `loanLockingService.applyLock()` method
- when there is an error set repeatStatus.CONTINUABLE
- for counter we might wanna use stepExecution commit count (with every run it got increased)
What do you think?
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy commented on a diff in pull request #3255: FINERACT-1724-COB-hardlocked-loans-and-inline-cob-issue
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3255:
URL: https://github.com/apache/fineract/pull/3255#discussion_r1229619742
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/LoanItemReader.java:
##########
@@ -56,7 +60,17 @@ public void beforeStep(@NotNull StepExecution stepExecution) {
loanIds = retrieveLoanIdService.retrieveAllNonClosedLoansByLastClosedBusinessDateAndMinAndMaxLoanId(loanCOBParameter,
customJobParameterResolver.getCustomJobParameterById(stepExecution, LoanCOBConstant.IS_CATCH_UP_PARAMETER_NAME)
.map(Boolean::parseBoolean).orElse(false));
+
+ List<Long> lockedByCOBChunkProcessingAccountIds = getLoanIdsLockedWithChunkProcessingLock(loanIds);
+ loanIds.retainAll(lockedByCOBChunkProcessingAccountIds);
}
setRemainingData(new ArrayList<>(loanIds));
}
+
+ private List<Long> getLoanIdsLockedWithChunkProcessingLock(List<Long> loanIds) {
+ List<LoanAccountLock> accountLocks = new ArrayList<>();
+ accountLocks.addAll(loanLockingService.findAllByLoanIdIn(loanIds));
Review Comment:
You might wanna do the filtering at database side
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy commented on a diff in pull request #3255: FINERACT-1724-COB-hardlocked-loans-and-inline-cob-issue
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3255:
URL: https://github.com/apache/fineract/pull/3255#discussion_r1229620690
##########
fineract-provider/src/main/java/org/apache/fineract/cob/service/InlineLoanCOBExecutorServiceImpl.java:
##########
@@ -218,8 +218,15 @@ private void lockLoanAccounts(List<Long> loanIds, LocalDate businessDate) {
protected void doInTransactionWithoutResult(@NotNull TransactionStatus status) {
List<LoanAccountLock> loanAccountLocks = getLoanAccountLocks(loanIds, businessDate);
loanAccountLocks.forEach(loanAccountLock -> {
- loanAccountLock.setNewLockOwner(LockOwner.LOAN_INLINE_COB_PROCESSING);
- loanAccountLockRepository.save(loanAccountLock);
+ try {
+ loanAccountLock.setNewLockOwner(LockOwner.LOAN_INLINE_COB_PROCESSING);
+ loanAccountLockRepository.saveAndFlush(loanAccountLock);
+ } catch (Exception e) {
+ String message = "There is a hard lock on the loan account without any error, so it can't be overruled. Locked loan ID: %s"
Review Comment:
This does not mean automatically this... i would rather go with a more generic error message...
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy commented on a diff in pull request #3255: FINERACT-1724-COB-hardlocked-loans-and-inline-cob-issue
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3255:
URL: https://github.com/apache/fineract/pull/3255#discussion_r1229609010
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/ApplyLoanLockTasklet.java:
##########
@@ -41,13 +43,43 @@
@RequiredArgsConstructor
public class ApplyLoanLockTasklet implements Tasklet {
+ private static final int NUMBER_OF_RETRIES = 3;
private final FineractProperties fineractProperties;
private final LoanLockingService loanLockingService;
private final RetrieveLoanIdService retrieveLoanIdService;
private final CustomJobParameterResolver customJobParameterResolver;
@Override
public RepeatStatus execute(@NotNull StepContribution contribution, @NotNull ChunkContext chunkContext) throws Exception {
+ // TODO: Need to check Spring Batch based retry logic in the future, with using RepeatStatus.CONTINUABLE to
+ // retry tasklet execution rather than do while. Currently with RepeatStatus.CONTINUABLE we are getting
+ // transaction rollback exception.
+ int numberOfExecutions = 0;
+ do {
+ try {
+ applyLoanLock(contribution);
+ return RepeatStatus.FINISHED;
+ } catch (Exception e) {
+ if (++numberOfExecutions > NUMBER_OF_RETRIES) {
+ String message = "There was an error applying lock to loan accounts.";
+ log.error("{}", message, e);
+ throw new LoanLockCannotBeAppliedException(message, e);
+ } else {
+ waitBeforeRetry();
+ }
+ }
+ } while (true);
+ }
+
+ private void waitBeforeRetry() {
+ try {
+ Thread.sleep(1000);
+ } catch (InterruptedException e) {
+ log.info("{}", "Error applying loan lock.", e);
Review Comment:
Should be rethrown the error here...and logging an error should be used the ERROR level.
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] adamsaghy commented on a diff in pull request #3255: FINERACT-1724-COB-hardlocked-loans-and-inline-cob-issue
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #3255:
URL: https://github.com/apache/fineract/pull/3255#discussion_r1229612781
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/ApplyLoanLockTasklet.java:
##########
@@ -66,14 +98,15 @@ public RepeatStatus execute(@NotNull StepContribution contribution, @NotNull Chu
List<LoanAccountLock> accountLocks = new ArrayList<>();
loanIdPartitions.forEach(loanIdPartition -> accountLocks.addAll(loanLockingService.findAllByLoanIdIn(loanIdPartition)));
- List<Long> alreadyLockedByChunkProcessingAccountIds = accountLocks.stream()
- .filter(e -> LockOwner.LOAN_COB_CHUNK_PROCESSING.equals(e.getLockOwner())).map(LoanAccountLock::getLoanId).toList();
+ Predicate<LoanAccountLock> isLocked = e -> LockOwner.LOAN_COB_CHUNK_PROCESSING.equals(e.getLockOwner())
+ || LockOwner.LOAN_INLINE_COB_PROCESSING.equals(e.getLockOwner());
List<Long> toBeProcessedLoanIds = new ArrayList<>(loanIds);
- toBeProcessedLoanIds.removeAll(alreadyLockedByChunkProcessingAccountIds);
+ List<Long> alreadyLockedByChunkProcessingOrInlineCOBAccountIds = accountLocks.stream().filter(isLocked)
Review Comment:
This can be removed. Every entry in the accountLocks means its locked and we dont need any filtering...
--
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: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org