Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FINERACT-849: Switch from OpenJPA to EclipseLink #2127

Merged
merged 1 commit into from Mar 10, 2022

Conversation

galovics
Copy link
Contributor

@galovics galovics commented Mar 8, 2022

Description

Describe the changes made and why they were made.

Ignore if these details are present on the associated Apache Fineract JIRA ticket.

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Write the commit message as per https://github.com/apache/fineract/#pull-requests

  • 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.

  • Create/update unit or integration tests for verifying the changes made.

  • Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.

  • 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

  • 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.

@vidakovic vidakovic self-requested a review March 8, 2022 19:10
@galovics galovics changed the title Fineract 849 rebased FINERACT-849: Switch from OpenJPA to EclipseLink Mar 8, 2022
@vidakovic
Copy link
Contributor

@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

@galovics
Copy link
Contributor Author

galovics commented Mar 9, 2022

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. 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.

@vidakovic
Copy link
Contributor

alright ... I think we can merge this one then.

Copy link
Contributor

@vidakovic vidakovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@xurror xurror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done.
LGTM too.

@vidakovic vidakovic merged commit d5c6115 into apache:develop Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants