vorburger commented on pull request #928: URL: https://github.com/apache/fineract/pull/928#issuecomment-650126648
Wow. First of all, this is impressive. Congratulations. I have some concerns that the bulk replace of `save()` by `saveAndFlush()` could actually lead to worse performance. About the disabled tests, I'm wondering whether we shouldn't fix those before merging this... one of them in particular (FINERACT-1051) seems to have something to do re. some "Pre-Closure maturity amount" being suddenly different, and nother one is a failing SQL (FINERACT-1050) and the third seems to indicate regression with the scheduler (FINERACT-1048), which with much pain we just fixed... it seems to me that if we merge this PR now, we knowingly break Fineract again, no? If in the meantime you already wanted to get a sub-set of this PR merged just to avoid future merge conflicts, e.g. those `JpaSystemException` (which is a Spring and not an EclipseLink specific class) you had to add could make a separate standalone PR which we could already merge before the rest of this (if you like, just a thought & suggestion). ---------------------------------------------------------------- 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: [email protected]
