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 2022/03/08 18:53:53 UTC
[GitHub] [fineract] galovics opened a new pull request #2127: Fineract 849 rebased
galovics opened a new pull request #2127:
URL: https://github.com/apache/fineract/pull/2127
## Description
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!
- [x] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [x] 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.
- [x] Create/update unit or integration tests for verifying the changes made.
- [x] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [x] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes
- [x] 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] vidakovic merged pull request #2127: FINERACT-849: Switch from OpenJPA to EclipseLink
Posted by GitBox <gi...@apache.org>.
vidakovic merged pull request #2127:
URL: https://github.com/apache/fineract/pull/2127
--
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] vidakovic commented on pull request #2127: FINERACT-849: Switch from OpenJPA to EclipseLink
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #2127:
URL: https://github.com/apache/fineract/pull/2127#issuecomment-1062746540
alright ... I think we can merge this one then.
--
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] galovics commented on pull request #2127: FINERACT-849: Switch from OpenJPA to EclipseLink
Posted by GitBox <gi...@apache.org>.
galovics commented on pull request #2127:
URL: https://github.com/apache/fineract/pull/2127#issuecomment-1062663276
thanks @vidakovic for the review, let me address your points:
* -Werror, the best would be to leave it there but EclipseLink simply generates bytecode that has warnings in it. There's a potential fix for this since the -Werror flag comes in when the compilation happens. We could separate the static weaving and the actual source code compilation into 2 different steps, like the regular compileJava (this would have -Werror enabled) and a staticWeaving step which has -Werror disabled. The thing is, in that case we have to play around the classpath configurations and generate the statically weaved code somewhere else than the regular build directory and there's a simple explanation for that. If I separate the static weaving and compilation into 2 different tasks and the static weaving overrides the existing build dir content, the Gradle up-to-date checks are going to be broken and every single time you try to restart the app from IntelliJ, it will recompile everything even though nothing changed.
* `DatabaseSelectingPersistenceUnitPostProcessor` very valid question. I needed to implement this last minute since I had the same impression. The current database support for Fineract is MySQL (and MariaDB) and PostgreSQL. The thing is, [EclipseLink only support MySQL and PostgreSQL but not MariaDB](https://www.eclipse.org/eclipselink/documentation/2.7/jpa/extensions/persistenceproperties_ref.htm#target-database). When I tried it with PostgreSQL, EclipseLink correctly recognized it's a PostgreSQL database but when I tried with MariaDB, it simply collapsed because it didn't recognize it as a MySQL database. I guess there's a difference what the JDBC driver says about itself when it's the MySQL driver or the MariaDB driver.
* `EntityScanningPersistenceUnitPostProcessor` That's needed otherwise the entity classes should be listed in the persistence.xml
* Good point, let me take a look there.
* Yeah, might look strange but the ID generation strategy is a bit different with EclipseLink than OpenJPA. If we try to get the primary key in the response to a request when we just persisted a new entity, first we gotta get the DB to generate the ID, hence the explicit flushing. This worked differently with OpenJPA.
* Agreed. Caching is a whole separate story which I want to cover later on. Logging, I was thinking the same but when I turned it on, it literally got unreadable so I might not want to expose this to be configurable via env vars. It's rather for development only in my mind.
Let me know your thoughts.
--
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] vidakovic commented on pull request #2127: FINERACT-849: Switch from OpenJPA to EclipseLink
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #2127:
URL: https://github.com/apache/fineract/pull/2127#issuecomment-1062273465
@galovics ... just some remarks (no show stoppers):
- removing "-Werror" is ok given the major benefit of using EclipseLink; maybe someone else can figure this out later... or we just relax a bit here (this flag is really harsh)
- not sure if I understand the need for "DatabaseSelectingPersistenceUnitPostProcessor"... I understand that it's used to select the database dialect, but isn't this already provided by EclipseLink?
- same with "EntityScanningPersistenceUnitPostProcessor"
- ... maybe I found my own answer to those 2 classes: could be replaced by using spring boot JPA starter package with auto configuration; alright, maybe for later in a separate PR; all good
- all the "saveAndFlush", ok, check
- we could make the persistence unit configuration a bit more convenient (read: configurable via env vars... especially the cache stuff might be a candidate for that or logging):
```
@Configuration
public class EclipseLinkJpaConfiguration extends JpaBaseConfiguration {
@Override
protected AbstractJpaVendorAdapter createJpaVendorAdapter() {
return new EclipseLinkJpaVendorAdapter();
}
@Override
protected Map<String, Object> getVendorProperties() {
HashMap<String, Object> map = new HashMap<>();
map.put(PersistenceUnitProperties.WEAVING, true);
map.put(PersistenceUnitProperties.DDL_GENERATION, "drop-and-create-tables");
map.put("eclipselink.persistence-context.close-on-commit", "true");
map.put("eclipselink.cache.shared.default", "false");
map.put("eclipselink.logging.level.sql", "FINE");
map.put("eclipselink.logging.parameters", "true");
return map;
}
//...
}
```
... and instead of the hard coded values we could add application.properties/env vars
--
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