[JDBC] : Deprecate EclipseLink

[singhpk234]

Deperecate Eclipse-link and make relational-jdbc as default

dev-list : https://lists.apache.org/thread/b2qx02hlvxl6rncb7l0xnnx0w22t3z9j

[dimas-b]

+1 to deprecate EclipseLink

nit: Eclipse-link -> EclipseLink in the title or perhaps eclipse-link Persistence?

[dimas-b]

From my POV application.properties is not the best place for user-level settings. In binary distributions the default one is embedded into jar files and not directly accessible, while the optional local file is relative to the current directory, which is prone to runtime mistakes.

I'd suggest using java -D options or env. variables in user-facing docs.

Cf. https://quarkus.io/guides/config-reference

[singhpk234]

I agree, as the whole page follows the theme of setting stuff in application.prop hence i kept this, please let me know what you think considering above !

[dimas-b]

I only read the diff, not the whole doc 😅 but I keep my point. We do not have to fix the whole doc in this PR, though.

[singhpk234]

No worries , how about i add another line below that in order to use the binaries configure using -D or env props ?

[eric-maynard]

Yeah this doc predates quarkus. Tbh I think it's kind of problematic that we don't offer a config file where you can change things without needing to rebuild. We should fix that experience even if we first update our docs to reflect the current (bad) state

[dimas-b]

In k8s it is pretty easy and natural to supply config as env. variables. However, if people like having the ability to use a local file for configuration, I guess it should not be too hard to support it with a predictable location (e.g. relative to the jar file or user's HOME dir as opposed to current dir). WDYT?

[jbonofre]

In another PR, I proposed to introduce a "user facing" configuration (it's very easy to do that with Quarkus) and it's always possible to override via env variables or config maps on k8s.

Short term in this PR, I would recommend to document the env variable use, and if you think it makes sense I can revive my PR.

[singhpk234]

documenting ENV vars makes sense to me, let me try doing that.

Re: if people like having the ability to use a local file for configuration

IMHO, quarkus is quite configurable and its the deployement time they would need, loading confs from file will introduce these complexity like fallbacks of loading this file from code never the less open the config override options for ex something configured in the file should override what the env specifies, by the time quarkus would get the handle of the file and apply configs, quarkus would have applied the confs to make DS ?

[singhpk234]

Here is how it looks :

[singhpk234]

+1 to reviving @jbonofre 's PR : https://github.com/apache/polaris/pull/1032/files#diff-7f7a4d7e028cff5774c249e58114c43abf6c983ad8be3fc955abec59865272ba

[eric-maynard]

I think we'll also have to look for other references to EclipseLink and persistence.xml, such as:

polaris/helm/polaris/README.md

Line 51 in 3b8aaff

When using a custom `persistence.xml`, a Kubernetes Secret must be created for it. Below is a sample command:

[pingtimeout]

Is this because all tests currently run against the EclipseLink implementation?

[dimas-b]

If we deprecate EclipseLink, why not purge it from tests too?

[jbonofre]

I think we can keep it for now (even deprecated) and remove when we remove eclipselink.

[dimas-b]

Let's add a separate test profile for EclipseLink then and run maybe a sub-set of tests. How about the bootstrap test in the admin tool and the REST API test in the quarkus server?

[dimas-b]

Note: removing this change actually makes all tests use JDBC, as far as I understand 🤔

[dimas-b]

It might be best to keep doc changes in this PR and make application.properties and test changes in another PR.

[singhpk234]

Let's add a separate test profile for EclipseLink then and run maybe a sub-set of tests. How about the bootstrap test in the admin tool and the REST API test in the quarkus server?

This profile is just used for eclipse link, the jdbc profile and lifecycle manager is is present is quarkus/test-commons. The reason why i kept both together is i we are deprecating eclipse link via doc and not via code, so hence updating default is in the same PR. Please Let me know your thoughts considering above.

[dimas-b]

ok. thx for the clarification!

[adnanhemani]

LGTM!

[adnanhemani]

Do users need to do any action here to make this happen? If not, would it be better to remove this line altogether?

[singhpk234]

yes, but the immediate line below re-directs the user to move to metastore.md page to configure the metastore, so this was kind a redundant. Hence removed.

[flyrain]

Thanks a lot for working on it, @singhpk234 ! LGTM overall. Left minor comments.

[flyrain]

Both env variable and Quarkus application.properties work. Can we be consistent about the configuration in the doc as the metastores.md use the application.properties way? I think we can mention the application.properties way here while refer to page https://polaris.apache.org/in-dev/unreleased/configuration/ if users prefer env variable. Thoughts?

[dimas-b]

I support consistency.

As to using application.properties, as I commented in another thread, they are relative to current dir and as such prone to errors, IMHO.

If we want to go with config files, I propose to revive @jbonofre PR on that.

[flyrain]

I'm OK with either way. Hi @dimas-b , can you share JB's PR?

[singhpk234]

+1 to revive JB's PR : https://github.com/apache/polaris/pull/1032/files#diff-7f7a4d7e028cff5774c249e58114c43abf6c983ad8be3fc955abec59865272ba

[flyrain]

I'm also supportive on JB's PR. It provides much better UX in terms of how users config Polaris server and admin tool. User can make changes to config/application.prooperties without searching for Polaris doc how to config things. We can even put certain common used configures in it, and comment them out by default, and users can uncomment them when needed.