You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/06/10 04:27:40 UTC
[GitHub] [fineract] ptuomola opened a new pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
ptuomola opened a new pull request #1024:
URL: https://github.com/apache/fineract/pull/1024
## Description
Rewrote the logic to remove call to rs.previous() that was throwing an SQLException.
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Commit message starts with the issue number from https://issues.apache.org/jira/projects/FINERACT/. Ex: FINERACT-646 Pockets API.
- [ ] Coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions have been followed.
- [ ] API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm has been updated with details of any API changes.
- [ ] Integration tests have been created/updated for verifying the changes made.
- [ ] All Integrations tests are passing with the new commits.
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the list.)
Our guidelines for code reviews is 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#issuecomment-646971968
/rebase
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#discussion_r443119349
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanArrearsAgingServiceImpl.java
##########
@@ -423,18 +423,14 @@ public OriginalScheduleExtractor(final String loanIdsAsString) {
while (rs.next()) {
Long loanId = rs.getLong("loanId");
- List<LoanSchedulePeriodData> periodDatas = new ArrayList<>();
- LoanSchedulePeriodData loanSchedulePeriodData = fetchLoanSchedulePeriodData(rs);
- periodDatas.add(loanSchedulePeriodData);
- while (rs.next()) {
- Long tempLoanId = rs.getLong("loanId");
- if (loanId.equals(tempLoanId)) {
Review comment:
@ptuomola sorry for delay in reviewing this (I wanted to do it with a clear mind instead of clicking through it half asleep a night after day job work...) and after staring at this a little bit, I get it now - this makes good sense to me as well, and I have no objection to merging this even without having any way to functionally test this myself either (no idea if existing ITs, recently re-enable, would catch this, but doesn't matter, it "makes sense").
One very small feedback I would have is that, now that I understand this logic, it seems to me that moving that `scheduleDate.put(loanId, periodDatas);` now in line 449 (originally 434) up into the `if(periodDatas == null)` right after the `periodDatas = new ArrayList<>();` would make it a tiny bit more clear and explicit what we're doing here? It's too minor to hold back merging this important fix, so I'll just go ahead and merge it ASAP, and let you raise a very small minor follow-up PR, if you like (or not, fine). _I originally wrote this before seeing the Spotless style failure, if you're going over this again, might as well do that too here - if you agree that makes sense?_
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#issuecomment-641713282
@vorburger @xurror - here's a quick rewrite to remove the call to rs.previous(). Unfortunately I have no sensible way of testing this - so no idea if it works! Does this look sensible and if yes, any ideas how we could test this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#discussion_r443157922
##########
File path: .travis.yml
##########
@@ -73,6 +73,7 @@ script:
# using "&&" instead of several "-" means that integrationTest does not run if test fails,
# and Docker test does not run if integration test fails, which makes PR failure easier to understand.
# @see https://docs.travis-ci.com/user/job-lifecycle/#customizing-the-build-phase
+# NOTE: Sleep after docker-compose increased to 60 seconds as often Travis would fail to get Docker up in 30 seconds
Review comment:
@ptuomola just noticed while merging this that you haven't actually changed the `sleep 30s` so this comment is perhaps a left over? I'll merge this PR anyway, you can always clean it up later, if you like.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#discussion_r439216301
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanArrearsAgingServiceImpl.java
##########
@@ -423,18 +423,14 @@ public OriginalScheduleExtractor(final String loanIdsAsString) {
while (rs.next()) {
Long loanId = rs.getLong("loanId");
- List<LoanSchedulePeriodData> periodDatas = new ArrayList<>();
- LoanSchedulePeriodData loanSchedulePeriodData = fetchLoanSchedulePeriodData(rs);
- periodDatas.add(loanSchedulePeriodData);
- while (rs.next()) {
- Long tempLoanId = rs.getLong("loanId");
- if (loanId.equals(tempLoanId)) {
Review comment:
Totally makes sense now. Thanks for helping me understand this better.
I actually read a comment that suggested doing this or using caching or hashmap or any other collection to hold all the resultset and instead iterate through it as you did but it just wasn't clear to me.
Unfortunately we don't have a way testing this but I agree with your approach. Since it passes all IT tests, we can just blindly merge this 2 or 3 days if there are no objections and hope for the best 😁.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] xurror commented on a change in pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
xurror commented on a change in pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#discussion_r439153425
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanArrearsAgingServiceImpl.java
##########
@@ -423,18 +423,14 @@ public OriginalScheduleExtractor(final String loanIdsAsString) {
while (rs.next()) {
Long loanId = rs.getLong("loanId");
- List<LoanSchedulePeriodData> periodDatas = new ArrayList<>();
- LoanSchedulePeriodData loanSchedulePeriodData = fetchLoanSchedulePeriodData(rs);
- periodDatas.add(loanSchedulePeriodData);
- while (rs.next()) {
- Long tempLoanId = rs.getLong("loanId");
- if (loanId.equals(tempLoanId)) {
Review comment:
I'm trying to understand this to the best I can but I just have the feeling that this piece of logic is missing. Can you maybe make it a little clearer for me?
Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger merged pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
vorburger merged pull request #1024:
URL: https://github.com/apache/fineract/pull/1024
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#discussion_r439197387
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanArrearsAgingServiceImpl.java
##########
@@ -423,18 +423,14 @@ public OriginalScheduleExtractor(final String loanIdsAsString) {
while (rs.next()) {
Long loanId = rs.getLong("loanId");
- List<LoanSchedulePeriodData> periodDatas = new ArrayList<>();
- LoanSchedulePeriodData loanSchedulePeriodData = fetchLoanSchedulePeriodData(rs);
- periodDatas.add(loanSchedulePeriodData);
- while (rs.next()) {
- Long tempLoanId = rs.getLong("loanId");
- if (loanId.equals(tempLoanId)) {
Review comment:
As far as I can see, the logic is trying to build a map of (loanId, periodData). There may be multiple rows in the ResultSet and these must therefore be merged to be part of the same entry in the map.
The previous code does it by:
1. Read one LoanId from resultset and create a map entry
2. In a loop, keep fetching next row from result set.
- If the next row has the same loanId, then add it to the same map entry and fetch next
- If the next row has a different loanId, go back one row in result set and start from step 1
The problem is the "go back one row" which is not supported by some JDBC drivers.
But an equivalent way of building the same map (at least in my view would be):
1. Iterate through the result set. For each row's loanId:
- If we already have a map entry for that loanId, add the dates to the existing entry
- If we don't yet have a map entry for that loanId, create a new one and add the dates to that
No rs.previous() required, and either way you should end up with the same Map - as far as I can see..
But I don't have a way to test this code, so a bit concerned I may have missed something...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#issuecomment-642905820
@awasum @fynmanoj @avikganguly01 @vishwasbabu are you able to help review this PR for FINERACT-995?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] awasum commented on pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
awasum commented on pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#issuecomment-643336812
I maybe able to look at this tomorrow..
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on a change in pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #1024:
URL: https://github.com/apache/fineract/pull/1024#discussion_r443119349
##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanArrearsAgingServiceImpl.java
##########
@@ -423,18 +423,14 @@ public OriginalScheduleExtractor(final String loanIdsAsString) {
while (rs.next()) {
Long loanId = rs.getLong("loanId");
- List<LoanSchedulePeriodData> periodDatas = new ArrayList<>();
- LoanSchedulePeriodData loanSchedulePeriodData = fetchLoanSchedulePeriodData(rs);
- periodDatas.add(loanSchedulePeriodData);
- while (rs.next()) {
- Long tempLoanId = rs.getLong("loanId");
- if (loanId.equals(tempLoanId)) {
Review comment:
@ptuomola sorry for delay in reviewing this (I wanted to do it with a clear mind instead of clicking through it half asleep a night after day job work...) and after staring at this a little bit, I get it now - this makes good sense to me as well, and I have no objection to merging this even without having any way to functionally test this myself either (no idea if existing ITs, recently re-enable, would catch this, but doesn't matter, it "makes sense").
One very small feedback I would have is that, now that I understand this logic, it seems to me that moving that `scheduleDate.put(loanId, periodDatas);` now in line 449 (originally 434) up into the `if(periodDatas == null)` right after the `periodDatas = new ArrayList<>();` would make it a tiny bit more clear and explicit what we're doing here? It's two minor to hold back merging this important fix, so I'll just go ahead and merged, and let you raise a very small minor follow-up PR, if you like (or not, fine).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org