FINERACT-995: Rewriting logic to remove call to rs.previous() #1024
Conversation
|
@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? |
|
@awasum @fynmanoj @avikganguly01 @vishwasbabu are you able to help review this PR for FINERACT-995? |
| periodDatas.add(loanSchedulePeriodData); | ||
| while (rs.next()) { | ||
| Long tempLoanId = rs.getLong("loanId"); | ||
| if (loanId.equals(tempLoanId)) { |
There was a problem hiding this 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.
There was a problem hiding this 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:
- Read one LoanId from resultset and create a map entry
- 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):
- 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...
There was a problem hiding this 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 😁.
There was a problem hiding this 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?
|
I maybe able to look at this tomorrow.. |
|
/rebase |
github-actions
bot
force-pushed
the
FINERACT-995
branch
from
558002c
to
311f861
Compare
There was a problem hiding this comment.
LGTM, but the rebase failed due to Spotless (probably the { needs to be on the same line as if..) - will you fix @ptuomola ?
Thank You for this contribution - that's one more release Blocker issue (FINERACT-995) done!
| periodDatas.add(loanSchedulePeriodData); | ||
| while (rs.next()) { | ||
| Long tempLoanId = rs.getLong("loanId"); | ||
| if (loanId.equals(tempLoanId)) { |
There was a problem hiding this 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?
| @@ -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 | |||
There was a problem hiding this 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.
FINERACT-995
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