Allow eclipselink config outside classpath #95

guitcastro · 2024-08-06T03:09:31Z

Description

As today, It's impossible to load configurations outside the classpath using eclipse-link. This PR allow do load config files from any path.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

I've update the PolarisEclipseLinkMetaStoreTest to ensure that the file is being loaded from outside the classpath.

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue
I have signed and submitted the ICLA and if needed, the CCLA. See Contributing for details.

…fig_outside_classpath

aihuaxu · 2024-08-06T03:58:40Z

...o/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java


    try {
-      InputStream input = this.getClass().getClassLoader().getResourceAsStream(confFile);
+      FileInputStream input = new FileInputStream(confFile);


This is fine but Persistence.createEntityManagerFactory(persistenceUnitName, properties) puts the limitation which can only loads the configuration from a jar. I'm making a change to workaround that. #88

I was able to fix this in the last commit. Please see the this comment in #79

I replied in another thread.

@guitcastro Actually it's not because of the properties. Of course we need the properties because we want to replace realm in the database name - each realm will have its own database.

Persistence.createEntityManagerFactory() will still load configuration file from resource even without properties.

@aihuaxu only if you specify the ECLIPSELINK_PERSISTENCE_XML property. I've tested and it worked.

From their docs:

This property is only used by EclipseLink when it is locating the configuration file.
When used within an EJB/Spring container in container managed mode the locating and reading of this file is done by the container and will not use this configuration.

I've added unit test to ensure config loads from outside the classpath is working.

@guitcastro Can you list the steps how you get it to work? It's possible the unit test found the persistence.xml from the embedded jar. That's why it doesn't fail for you.

I don't see the changes or unit tests in the PR. Did you forget to include that?

I only see
properties.put(ECLIPSELINK_PERSISTENCE_XML, confFile); -- removed

FileInputStream input = new FileInputStream(confFile);

@aihuaxu Sorry, I have forgot to push it. Can you please take a look now? I also added a test to ensure that the config is not being loaded from the classpath.

eric-maynard · 2024-08-06T04:57:58Z

This looks similar to #79, which did not work when tested.

guitcastro · 2024-08-06T17:27:38Z

This looks similar to #79, which did not work when tested.

I fix this in the last commit. Please see the this comment in #79

eric-maynard · 2024-08-06T19:01:35Z

Hi @guitcastro, I tested this change and found that it did not work. See my notes below:

I copied the default persistence.xml to my desktop and updated the polaris-server.yml to point to it:

. . .
metaStoreManager:
  type: eclipse-link
  persistence-unit: polaris-dev
  conf-file: /Users/emaynard/Desktop/persistence.xml
. . .

I then modified the default persistence.xml at .../resources/META-INF/persistence.xml to use a different persistence-unit -- polaris-fake instead of polaris-dev. When I attempt to bootstrap, I see this:

jakarta.persistence.PersistenceException: No Persistence provider for EntityManager named polaris-dev
	at jakarta.persistence.Persistence.createEntityManagerFactory(Persistence.java:86)
	at io.polaris.extension.persistence.impl.eclipselink.PolarisEclipseLinkMetaStoreSessionImpl.<init>(PolarisEclipseLinkMetaStoreSessionImpl.java:109)
. . .

If I revert the default persistence.xml back to using polaris-dev, it bootstraps normally.

Based on this, I suspect that your change is in fact just loading the default persistence.xml which is on the classpath, and that it does not actually allow us to load a persistence.xml outside of the classpath. This makes sense when you consider your change is deleting the config that is supposed to point to the location of the persistence.xml. Is it possible you were observing the same in your testing?

guitcastro · 2024-08-06T19:18:31Z

Hi @guitcastro, I tested this change and found that it did not work. See my notes below:

I copied the default persistence.xml to my desktop and updated the polaris-server.yml to point to it:
. . .
metaStoreManager:
  type: eclipse-link
  persistence-unit: polaris-dev
  conf-file: /Users/emaynard/Desktop/persistence.xml
. . .
I then modified the default persistence.xml at .../resources/META-INF/persistence.xml to use a different persistence-unit -- polaris-fake instead of polaris-dev. When I attempt to bootstrap, I see this:
jakarta.persistence.PersistenceException: No Persistence provider for EntityManager named polaris-dev
	at jakarta.persistence.Persistence.createEntityManagerFactory(Persistence.java:86)
	at io.polaris.extension.persistence.impl.eclipselink.PolarisEclipseLinkMetaStoreSessionImpl.<init>(PolarisEclipseLinkMetaStoreSessionImpl.java:109)
. . .
If I revert the default persistence.xml back to using polaris-dev, it bootstraps normally.

Based on this, I suspect that your change is in fact just loading the default persistence.xml which is on the classpath, and that it does not actually allow us to load a persistence.xml outside of the classpath. This makes sense when you consider your change is deleting the config that is supposed to point to the location of the persistence.xml. Is it possible you were observing the same in your testing?

Maybe the /Users/emaynard/Desktop/persistence.xml is not defining a polaris-dev. I just commit two unit test that ensure that the default config is not being loaded. If you test the lasted version it should at least throw an exception instead of loading the default one.

eric-maynard · 2024-08-06T19:41:48Z

...java/com/snowflake/polaris/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreTest.java

+  }
+
+  @Test
+  void ensureNotLoadingDefaultClassPersistence() {


How does this test that you're not still loading the file from /META-INF/persistence.xml?

eric-maynard · 2024-08-06T19:42:23Z

No @guitcastro it was definitely there -- like I mentioned, I copied it directly from the existing default file. Have you done an end-to-end test using this change including bootstrapping Polaris? If so can you share the steps?

aihuaxu · 2024-08-06T19:42:58Z

...java/com/snowflake/polaris/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreTest.java

    PolarisEclipseLinkMetaStoreSessionImpl session =
        new PolarisEclipseLinkMetaStoreSessionImpl(
-            store, Mockito.mock(), () -> "realm", null, "polaris-dev");
+            store, Mockito.mock(), () -> "realm", tmpFile.toString(), "polaris-dev");


For this, Persistence.createEntityManagerFactory() will use the one in the jar, not the /tmp/%s-persistence.xml.

You can try to remove persistence.xml from the project and see the issue.

guitcastro · 2024-08-06T21:26:54Z

@aihuaxu @eric-maynard You are right, what happened is that my external configuration had a persistence unit also named polaris-dev and got merged with the one in the classpath. Thus my jakarta.persistence.jdbc.url took precedence from the class path and I was able to connect with my database (postgres).

sfc-gh-aixu · 2024-08-06T22:36:56Z

@aihuaxu @eric-maynard You are right, what happened is that my external configuration had a persistence unit also named polaris-dev and got merged with the one in the classpath. Thus my jakarta.persistence.jdbc.url took precedence from the class path and I was able to connect with my database (postgres).

Yeah. So let's move forward with my workaround solution for now until we have a better one?

* chore(deps): update dependency mypy to >=1.17, <=1.17.0 (apache#2114) * Spark 3.5.6 and Iceberg 1.9.1 (apache#1960) * Spark 3.5.6 and Iceberg 1.9.1 * Cleanup * Add `pathStyleAccess` to AwsStorageConfigInfo (apache#2012) * Add `pathStyleAccess` to AwsStorageConfigInfo This change allows configuring the "path-style" access mode in S3 clients (both in Polaris Servers and Iceberg REST Catalog API clients). This change is applicable both to AWS storage and to non-AWS S3-compatible storage (apache#1530). * Add TestFileIOFactory helper (apache#2105) * Add FileIOFactory.wrapExisting helper * fix(deps): update dependency gradle.plugin.org.jetbrains.gradle.plugin.idea-ext:gradle-idea-ext to v1.2 (apache#2125) * fix(deps): update dependency boto3 to v1.39.7 (apache#2124) * Abstract polaris-runtime-service tests for all persistence implementations (apache#2106) The NoSQL persistence implementation has to run the Iceberg table & view catalog plus the Polaris specific tests as well. Reusing existing tests is beneficial to avoid a lot of code duplcation. This change moves the actual tests to `Abstract*` classes and refactors the existing tests to extend those. The NoSQL persistence work extends the same `Abstract*` classes but runs with different Quarkus test profiles. * Add IMPLICIT authentication support to the CLI (apache#2121) PRs apache#1925 and apache#1912 were merged around the same time. This PR connects the two changes and enables the CLI to accept IMPLICIT authentication type. Since Hadoop federated catalogs rely purely on IMPLICIT authentication, the CLI parsing test has been updated to reflect the same. * feat(helm): Add support for external authentication (apache#2104) * fix(deps): update dependency org.apache.iceberg:iceberg-bom to v1.9.2 (apache#2126) * fix(deps): update quarkus platform and group to v3.24.4 (apache#2128) * fix(deps): update dependency boto3 to v1.39.8 (apache#2129) * fix(deps): update dependency io.smallrye.config:smallrye-config-core to v3.13.3 (apache#2130) * Add newIcebergCatalog helper (apache#2134) creation of `IcebergCatalog` instances was quite redundant as tests mostly use the same parameters most of the time. also remove an unused field in 2 other tests. * Add server and client support for the new generic table `baseLocation` field (apache#2122) * Use Makefile to simplify setup and commands (apache#2027) * Use Makefile to simplify setup and commands * Add targets for minikube state management * Add podman support and spark plugin build * Add version target * Update README.md for Makefile usage and relation to the project * Fix nit * Package polaris client as python package (apache#2049) * Package polaris client as python package * Package polaris client as python package * Change owner to spark when copying files from local into Dockerfile * CI: Address failure from accessing GH API (apache#2132) CI sometimes fails with this failure: ``` * What went wrong: Execution failed for task ':generatePomFileForMavenPublication'. > Unable to process url: https://api.github.com/repos/apache/polaris/contributors?per_page=1000 ``` The sometimes failing request fetches the list of contributors to be published in the "root" POM. Unauthorized GH API requests have an hourly(?) limit of 60 requests per source IP. Authorized requests have a much higher rate limit. We do have a GitHub token available in every CI run, which can be used in GH API requests. This change adds the `Authorization` header for the failing GH API request to leverage the higher rate limit and let CI not fail (that often). * fix(deps): update dependency com.nimbusds:nimbus-jose-jwt to v10.4 (apache#2139) * fix(deps): update dependency com.diffplug.spotless:spotless-plugin-gradle to v7.2.0 (apache#2142) * fix(deps): update dependency software.amazon.awssdk:bom to v2.32.4 (apache#2146) * fix(deps): update dependency org.xerial.snappy:snappy-java to v1.1.10.8 (apache#2138) * fix(deps): update dependency org.junit:junit-bom to v5.13.4 (apache#2147) * fix(deps): update dependency boto3 to v1.39.9 (apache#2137) * fix(deps): update dependency com.fasterxml.jackson:jackson-bom to v2.19.2 (apache#2136) * Python client: add support for endpoint, sts-endpoint, path-style-access (apache#2127) This change adds support for endpoint, sts-endpoint, path-style-access to the Polaris Python client. Amends apache#1913 and apache#2012 * Remove PolarisEntityManager.getCredentialCache (apache#2133) `PolarisEntityManager` itself is not using the `StorageCredentialCache` but just hands it out via `getCredentialCache`. the only caller of `getCredentialCache` is `FileIOUtil.refreshAccessConfig`, which in in turn is only called by `DefaultFileIOFactory` and `IcebergCatalog`. note that in a follow-up we will likely be able to remove `PolarisEntityManager` usage completely from `IcebergCatalog`. additional cleanups: - use `StorageCredentialCache` injection in tests (but we need to invalidate all entries on test start) - remove unused `UserSecretsManagerFactory` from `PolarisCallContextCatalogFactory` * chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.22-1.1752676419 (apache#2150) * fix(deps): update dependency com.diffplug.spotless:spotless-plugin-gradle to v7.2.1 (apache#2152) * fix(deps): update dependency boto3 to v1.39.10 (apache#2151) * chore: fix class reference in the javadoc of TableLikeEntity (apache#2157) * fix(deps): update dependency commons-codec:commons-codec to v1.19.0 (apache#2160) * fix(deps): update dependency boto3 to v1.39.11 (apache#2159) * Last merged commit 395459f --------- Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Pooja Nilangekar <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: Yun Zou <[email protected]>

guilhermecastro added 2 commits August 6, 2024 00:01

allow load eclipse-link configs from outside classpath

27bc0e1

Merge remote-tracking branch 'origin/main' into allow_eclipselink_con…

9a05ce4

…fig_outside_classpath

guitcastro requested a review from a team as a code owner August 6, 2024 03:09

aihuaxu reviewed Aug 6, 2024

View reviewed changes

removed ECLIPSELINK_PERSISTENCE_XML property

cfc596c

guitcastro mentioned this pull request Aug 6, 2024

Support external config file for eclipselink #88

Merged

15 tasks

aihuaxu mentioned this pull request Aug 6, 2024

Support user-specified paths for conf-file in the eclipse-link metastore manager #79

Closed

9 tasks

guilhermecastro added 2 commits August 6, 2024 16:12

unit test

bfe2286

add test to ensure that is not loading the default class path

13831f7

eric-maynard reviewed Aug 6, 2024

View reviewed changes

aihuaxu reviewed Aug 6, 2024

View reviewed changes

guitcastro closed this Aug 7, 2024

adutra mentioned this pull request Jan 14, 2025

EclipseLink: allow reading persistence.xml from the filesystem #613

Merged

Allow eclipselink config outside classpath #95

Allow eclipselink config outside classpath #95

Uh oh!

Conversation

guitcastro commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guitcastro Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aihuaxu Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guitcastro commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-maynard commented Aug 6, 2024

Uh oh!

guitcastro commented Aug 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard commented Aug 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guitcastro commented Aug 6, 2024

Uh oh!

sfc-gh-aixu commented Aug 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guitcastro commented Aug 6, 2024 •

edited

Loading

guitcastro Aug 6, 2024 •

edited

Loading

aihuaxu Aug 6, 2024 •

edited

Loading

eric-maynard commented Aug 6, 2024 •

edited

Loading

guitcastro commented Aug 6, 2024 •

edited

Loading