You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "galovics (via GitHub)" <gi...@apache.org> on 2023/04/12 09:08:51 UTC
[GitHub] [fineract] galovics commented on a diff in pull request #3112: [FINERACT-1678] Exclude loan IDs from execution context
galovics commented on code in PR #3112:
URL: https://github.com/apache/fineract/pull/3112#discussion_r1163839383
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/FetchAndLockLoanTasklet.java:
##########
@@ -46,67 +40,44 @@ public class FetchAndLockLoanTasklet implements Tasklet {
private static final Long NUMBER_OF_DAYS_BEHIND = 1L;
- private final LoanAccountLockRepository loanAccountLockRepository;
-
private final RetrieveLoanIdService retrieveLoanIdService;
- private final FineractProperties fineractProperties;
-
private final JdbcTemplate jdbcTemplate;
@Override
public RepeatStatus execute(@NotNull StepContribution contribution, @NotNull ChunkContext chunkContext) throws Exception {
String businessDateParameter = (String) contribution.getStepExecution().getJobExecution().getExecutionContext()
.get(LoanCOBConstant.BUSINESS_DATE_PARAMETER_NAME);
LocalDate businessDate = LocalDate.parse(Objects.requireNonNull(businessDateParameter));
- List<Long> allNonClosedLoanIds = retrieveLoanIdService.retrieveLoanIdsNDaysBehind(NUMBER_OF_DAYS_BEHIND, businessDate);
- if (allNonClosedLoanIds.isEmpty()) {
- contribution.getStepExecution().getJobExecution().getExecutionContext().put(LoanCOBConstant.LOAN_IDS, Collections.emptyList());
+ LoanCOBMinANdMaxLoanId minAndMaxLoanId = retrieveLoanIdService.retrieveMinAndMaxLoanIdsNDaysBehind(NUMBER_OF_DAYS_BEHIND,
Review Comment:
I think responsibility-wise, this shouldn't be here. Why don't we create a separate tasklet before this one?
##########
fineract-provider/src/main/java/org/apache/fineract/cob/data/LoanCOBMinANdMaxLoanId.java:
##########
@@ -0,0 +1,34 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.cob.data;
+
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import lombok.AllArgsConstructor;
+import lombok.Getter;
+import lombok.NoArgsConstructor;
+
+@AllArgsConstructor
+@Getter
+@NoArgsConstructor
+@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS)
+public class LoanCOBMinANdMaxLoanId {
Review Comment:
Why don't we call it what it's used for not what it stores?
For example LoanCOBInput or LoanCOBParameter or smthing like that.
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/ApplyLoanLockTasklet.java:
##########
@@ -46,11 +50,15 @@ public class ApplyLoanLockTasklet implements Tasklet {
private final LoanAccountLockRepository accountLockRepository;
private final FineractProperties fineractProperties;
private final JdbcTemplate jdbcTemplate;
+ private final LoanRepository loanRepository;
@Override
public RepeatStatus execute(@NotNull StepContribution contribution, @NotNull ChunkContext chunkContext) throws Exception {
ExecutionContext executionContext = contribution.getStepExecution().getExecutionContext();
- List<Long> loanIds = (List<Long>) executionContext.get(LoanCOBConstant.LOAN_IDS);
+ LoanCOBMinANdMaxLoanId loanCOBMinANdMaxLoanId = (LoanCOBMinANdMaxLoanId) executionContext.get(LoanCOBConstant.LOAN_IDS);
Review Comment:
As you can see, the LoanCOB constant you used here is LOAN_IDS, but that's kinda confusing since it's not Loan IDs anymore. That's why I suggested having a class called LoanCOBInput; representing all the input values for the COB makes more sense.
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/ApplyLoanLockTasklet.java:
##########
@@ -46,11 +50,15 @@ public class ApplyLoanLockTasklet implements Tasklet {
private final LoanAccountLockRepository accountLockRepository;
private final FineractProperties fineractProperties;
private final JdbcTemplate jdbcTemplate;
+ private final LoanRepository loanRepository;
@Override
public RepeatStatus execute(@NotNull StepContribution contribution, @NotNull ChunkContext chunkContext) throws Exception {
ExecutionContext executionContext = contribution.getStepExecution().getExecutionContext();
- List<Long> loanIds = (List<Long>) executionContext.get(LoanCOBConstant.LOAN_IDS);
+ LoanCOBMinANdMaxLoanId loanCOBMinANdMaxLoanId = (LoanCOBMinANdMaxLoanId) executionContext.get(LoanCOBConstant.LOAN_IDS);
+ List<Long> loanIds = new ArrayList<>(
+ loanRepository.findAllNonClosedLoansBehindOrNullByMinAndMaxLoanId(loanCOBMinANdMaxLoanId.getMinLoanId(),
Review Comment:
I think that's reasonable because if the min/max values are not passed down through the context, we're in a huge pile of ... :)
The only thing we could do is to throw a different exception in case this is null but doesn't add much value imho.
##########
fineract-provider/src/main/java/org/apache/fineract/cob/loan/FetchAndLockLoanTasklet.java:
##########
@@ -46,67 +40,44 @@ public class FetchAndLockLoanTasklet implements Tasklet {
private static final Long NUMBER_OF_DAYS_BEHIND = 1L;
- private final LoanAccountLockRepository loanAccountLockRepository;
-
private final RetrieveLoanIdService retrieveLoanIdService;
- private final FineractProperties fineractProperties;
-
private final JdbcTemplate jdbcTemplate;
@Override
public RepeatStatus execute(@NotNull StepContribution contribution, @NotNull ChunkContext chunkContext) throws Exception {
String businessDateParameter = (String) contribution.getStepExecution().getJobExecution().getExecutionContext()
.get(LoanCOBConstant.BUSINESS_DATE_PARAMETER_NAME);
LocalDate businessDate = LocalDate.parse(Objects.requireNonNull(businessDateParameter));
- List<Long> allNonClosedLoanIds = retrieveLoanIdService.retrieveLoanIdsNDaysBehind(NUMBER_OF_DAYS_BEHIND, businessDate);
- if (allNonClosedLoanIds.isEmpty()) {
- contribution.getStepExecution().getJobExecution().getExecutionContext().put(LoanCOBConstant.LOAN_IDS, Collections.emptyList());
+ LoanCOBMinANdMaxLoanId minAndMaxLoanId = retrieveLoanIdService.retrieveMinAndMaxLoanIdsNDaysBehind(NUMBER_OF_DAYS_BEHIND,
+ businessDate);
+ if (Objects.isNull(minAndMaxLoanId)
+ || (Objects.isNull(minAndMaxLoanId.getMinLoanId()) && Objects.isNull(minAndMaxLoanId.getMaxLoanId()))) {
+ contribution.getStepExecution().getJobExecution().getExecutionContext().put(LoanCOBConstant.LOAN_IDS, null);
return RepeatStatus.FINISHED;
}
- List<Long> remainingIds = new ArrayList<>(allNonClosedLoanIds);
-
- List<List<Long>> remainingIdPartitions = Lists.partition(remainingIds, getInClauseParameterSizeLimit());
- List<LoanAccountLock> loanAccountLocks = new ArrayList<>();
- remainingIdPartitions.forEach(
- remainingIdPartition -> loanAccountLocks.addAll(loanAccountLockRepository.findAllByLoanIdIn(remainingIdPartition)));
-
- List<Long> alreadySoftLockedAccounts = loanAccountLocks.stream()
- .filter(e -> LockOwner.LOAN_COB_PARTITIONING.equals(e.getLockOwner())).map(LoanAccountLock::getLoanId).toList();
- List<Long> alreadyMarkedForInlineCOBLockedAccounts = loanAccountLocks.stream()
- .filter(e -> LockOwner.LOAN_INLINE_COB_PROCESSING.equals(e.getLockOwner())).map(LoanAccountLock::getLoanId).toList();
- List<Long> alreadyMarkedForChunkProcessingLockedAccounts = loanAccountLocks.stream()
- .filter(e -> LockOwner.LOAN_COB_CHUNK_PROCESSING.equals(e.getLockOwner())).map(LoanAccountLock::getLoanId).toList();
- // Remove already hard locked accounts
- remainingIds.removeAll(alreadyMarkedForChunkProcessingLockedAccounts);
- remainingIds.removeAll(alreadyMarkedForInlineCOBLockedAccounts);
+ applySoftLock(businessDate);
- List<Long> lockableLoanAccounts = new ArrayList<>(remainingIds);
- lockableLoanAccounts.removeAll(alreadySoftLockedAccounts);
-
- applySoftLock(lockableLoanAccounts);
-
- contribution.getStepExecution().getJobExecution().getExecutionContext().put(LoanCOBConstant.LOAN_IDS, remainingIds);
+ contribution.getStepExecution().getJobExecution().getExecutionContext().put(LoanCOBConstant.LOAN_IDS, minAndMaxLoanId);
return RepeatStatus.FINISHED;
}
- private void applySoftLock(List<Long> accountsToLock) {
+ private void applySoftLock(LocalDate businessDate) {
LocalDate cobBusinessDate = ThreadLocalContextUtil.getBusinessDateByType(BusinessDateType.COB_DATE);
- jdbcTemplate.batchUpdate("""
+ jdbcTemplate.update("""
Review Comment:
Aren't we missing the minLoanId and maxLoanId range filter in this query? In case there's a loan getting created between retrieving the min/max loanIds and the soft locking. The soft locking will take that loan too, but the min/max range will not include it, hence the COB will not pick it up.
--
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