You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2025/01/06 17:06:27 UTC
[PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
adutra opened a new pull request, #613:
URL: https://github.com/apache/polaris/pull/613
I turns out it is actually possible to read a persistence.xml file directly from the filesystem.
The trick is that it must be in a directory that will be considered as the "archive directory"; the `eclipselink.persistencexml` property _must_ point to the path of the file relative to the archive directory's parent. E.g. if the persistence file is in `/config/persistence.xml`, the archive directory is `/config` and the `eclipselink.persistencexml` property must be `config/persistence.xml`.
This PR builds on that observation and expands the allowed locations for a persistence.xml file to the following ones:
1. Classpath resource
2. Classpath resource, embedded in a jar
3. Filesystem path
4. Filesystem path, embedded in a jar
Note: this PR does not modify the Helm chart, but we could look into simplifying it to use plain files instead of jars.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #613:
URL: https://github.com/apache/polaris/pull/613#issuecomment-2590588296
Related: #95.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #613:
URL: https://github.com/apache/polaris/pull/613#discussion_r1904457937
##########
extension/persistence/eclipselink/src/test/resources/org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar:
##########
Review Comment:
I do think in the future we should be making more stuff at run/test time (e.g. the python client code can be generated from the spec). But I agree that doesn't need to block this fix
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard merged PR #613:
URL: https://github.com/apache/polaris/pull/613
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #613:
URL: https://github.com/apache/polaris/pull/613#discussion_r1904431994
##########
extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java:
##########
@@ -204,11 +208,39 @@ void testRotateLegacyPrincipalSecret() {
private static class CreateStoreSessionArgs implements ArgumentsProvider {
@Override
- public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+ public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext)
+ throws URISyntaxException {
+ Path persistenceXml =
+ Paths.get(
+ Objects.requireNonNull(getClass().getResource("/META-INF/persistence.xml")).toURI());
+ Path confJar =
+ Paths.get(
+ Objects.requireNonNull(
+ getClass()
+ .getResource(
+ "/org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar"))
+ .toURI());
return Stream.of(
+ // conf file not provided
+ Arguments.of(null, true),
+ // classpath resource
Arguments.of("META-INF/persistence.xml", true),
- Arguments.of("./build/conf/conf.jar!/persistence.xml", true),
- Arguments.of("/dummy_path/conf.jar!/persistence.xml", false));
+ Arguments.of("META-INF/dummy.xml", false),
+ // classpath resource, embedded
+ Arguments.of(
+ "org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar!/persistence.xml",
+ true),
+ Arguments.of(
+ "org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar!/dummy.xml",
+ false),
+ Arguments.of("dummy/test-conf.jar!/persistence.xml", false),
+ // filesystem path
+ Arguments.of(persistenceXml.toString(), true),
+ Arguments.of("/dummy_path/conf/persistence.xml", false),
+ // filesystem path, embedded
+ Arguments.of(confJar + "!/persistence.xml", true),
+ Arguments.of(confJar + "!/dummy.xml", false),
Review Comment:
Yes, it's my understanding reading the code: it's "backward" compatible, fallbacking to the "inner" persistence xml.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #613:
URL: https://github.com/apache/polaris/pull/613#discussion_r1904436808
##########
extension/persistence/eclipselink/src/test/resources/org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar:
##########
Review Comment:
Could it be made at build time?.. or perhaps by test code itself?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #613:
URL: https://github.com/apache/polaris/pull/613#discussion_r1904454500
##########
extension/persistence/eclipselink/src/test/resources/org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar:
##########
Review Comment:
My concern is that _if_ EclipseLink config evolves, the jar would go out of sync with the production code... But this is not likely, I guess... I think it's ok to merge "as is",
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #613:
URL: https://github.com/apache/polaris/pull/613#discussion_r1904427645
##########
extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java:
##########
@@ -204,11 +208,39 @@ void testRotateLegacyPrincipalSecret() {
private static class CreateStoreSessionArgs implements ArgumentsProvider {
@Override
- public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+ public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext)
+ throws URISyntaxException {
+ Path persistenceXml =
+ Paths.get(
+ Objects.requireNonNull(getClass().getResource("/META-INF/persistence.xml")).toURI());
+ Path confJar =
+ Paths.get(
+ Objects.requireNonNull(
+ getClass()
+ .getResource(
+ "/org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar"))
+ .toURI());
return Stream.of(
+ // conf file not provided
+ Arguments.of(null, true),
+ // classpath resource
Arguments.of("META-INF/persistence.xml", true),
- Arguments.of("./build/conf/conf.jar!/persistence.xml", true),
- Arguments.of("/dummy_path/conf.jar!/persistence.xml", false));
+ Arguments.of("META-INF/dummy.xml", false),
+ // classpath resource, embedded
+ Arguments.of(
+ "org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar!/persistence.xml",
+ true),
+ Arguments.of(
+ "org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar!/dummy.xml",
+ false),
+ Arguments.of("dummy/test-conf.jar!/persistence.xml", false),
+ // filesystem path
+ Arguments.of(persistenceXml.toString(), true),
+ Arguments.of("/dummy_path/conf/persistence.xml", false),
+ // filesystem path, embedded
+ Arguments.of(confJar + "!/persistence.xml", true),
+ Arguments.of(confJar + "!/dummy.xml", false),
Review Comment:
To make sure I'm reading these correctly, this means the new code should work with existing config + jar setups?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #613:
URL: https://github.com/apache/polaris/pull/613#discussion_r1904456726
##########
extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java:
##########
@@ -204,11 +208,39 @@ void testRotateLegacyPrincipalSecret() {
private static class CreateStoreSessionArgs implements ArgumentsProvider {
@Override
- public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+ public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext)
+ throws URISyntaxException {
+ Path persistenceXml =
+ Paths.get(
+ Objects.requireNonNull(getClass().getResource("/META-INF/persistence.xml")).toURI());
+ Path confJar =
+ Paths.get(
+ Objects.requireNonNull(
+ getClass()
+ .getResource(
+ "/org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar"))
+ .toURI());
return Stream.of(
+ // conf file not provided
+ Arguments.of(null, true),
+ // classpath resource
Arguments.of("META-INF/persistence.xml", true),
- Arguments.of("./build/conf/conf.jar!/persistence.xml", true),
- Arguments.of("/dummy_path/conf.jar!/persistence.xml", false));
+ Arguments.of("META-INF/dummy.xml", false),
+ // classpath resource, embedded
+ Arguments.of(
+ "org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar!/persistence.xml",
+ true),
+ Arguments.of(
+ "org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar!/dummy.xml",
+ false),
+ Arguments.of("dummy/test-conf.jar!/persistence.xml", false),
+ // filesystem path
+ Arguments.of(persistenceXml.toString(), true),
+ Arguments.of("/dummy_path/conf/persistence.xml", false),
+ // filesystem path, embedded
+ Arguments.of(confJar + "!/persistence.xml", true),
+ Arguments.of(confJar + "!/dummy.xml", false),
Review Comment:
Awesome! Great find
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #613:
URL: https://github.com/apache/polaris/pull/613#discussion_r1904436436
##########
extension/persistence/eclipselink/src/test/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreManagerTest.java:
##########
@@ -204,11 +208,39 @@ void testRotateLegacyPrincipalSecret() {
private static class CreateStoreSessionArgs implements ArgumentsProvider {
@Override
- public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+ public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext)
+ throws URISyntaxException {
+ Path persistenceXml =
+ Paths.get(
+ Objects.requireNonNull(getClass().getResource("/META-INF/persistence.xml")).toURI());
+ Path confJar =
+ Paths.get(
+ Objects.requireNonNull(
+ getClass()
+ .getResource(
+ "/org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar"))
+ .toURI());
return Stream.of(
+ // conf file not provided
+ Arguments.of(null, true),
+ // classpath resource
Arguments.of("META-INF/persistence.xml", true),
- Arguments.of("./build/conf/conf.jar!/persistence.xml", true),
- Arguments.of("/dummy_path/conf.jar!/persistence.xml", false));
+ Arguments.of("META-INF/dummy.xml", false),
+ // classpath resource, embedded
+ Arguments.of(
+ "org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar!/persistence.xml",
+ true),
+ Arguments.of(
+ "org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar!/dummy.xml",
+ false),
+ Arguments.of("dummy/test-conf.jar!/persistence.xml", false),
+ // filesystem path
+ Arguments.of(persistenceXml.toString(), true),
+ Arguments.of("/dummy_path/conf/persistence.xml", false),
+ // filesystem path, embedded
+ Arguments.of(confJar + "!/persistence.xml", true),
+ Arguments.of(confJar + "!/dummy.xml", false),
Review Comment:
Yes, it's backwards-compatible.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] EclipseLink: allow reading persistence.xml from the filesystem [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #613:
URL: https://github.com/apache/polaris/pull/613#discussion_r1904443161
##########
extension/persistence/eclipselink/src/test/resources/org/apache/polaris/extension/persistence/impl/eclipselink/test-conf.jar:
##########
Review Comment:
Certainly... it's just 1.5Kb though, is that worth the hassle?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org