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

Add extra docs about using EclipseLink #195

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

danielvdende
Copy link

Description

This PR adds a little bit of information in the docs on how to get up and running with EclipseLink. Perhaps this is obvious to experienced EclipseLink users but for newbies (like me) to EclipseLInk this was not clear.

Type of change

Please delete options that are not relevant.

  • This change requires a documentation update

How Has This Been Tested?

not applicable

Checklist:

Please delete options that are not relevant.

  • I have performed a self-review of my code

Comment on lines 58 to 64
> [!IMPORTANT]
> To use [EclipseLink](https://eclipse.dev/eclipselink), you need to ensure that you have done two things:
> 1. Build the jar for the EclipseLink extension
> 2. Set the property on the Polaris project to include eclipseLink.
>
> This can be achieved by setting `eclipseLink=true` in the `gradle.properties` file, or by passing the property explicitly while building all jars, e.g.: `./gradlew -PeclipseLink=true clean assemble`

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need something like this for sure. But I don't think this is really connected to configuring Polaris, but building it. Maybe there's a better place for this, or a new doc we need to write?

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. I added it here because this is where I found myself looking for more information. Do you have any pointers/ideas on where it would fit better?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's okay to have it here, approved for now with a few comments. In the future it might be neat to have a doc enumerating the different MetastoreManager options, and we could link to that doc from here.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @eric-maynard. I've made the changes as per your comments.

@@ -55,6 +55,13 @@ Be sure to secure your metastore backend since it will be storing credentials an

To use [EclipseLink](https://eclipse.dev/eclipselink/) for metastore management, specify the configuration `metaStoreManager.conf-file` to point to an [EclipseLink `persistence.xml` file](https://eclipse.dev/eclipselink/documentation/2.5/solutions/testingjpa002.htm). This file, local to the Polaris service, will contain information on what database to use for metastore management and how to connect to it.

> [!IMPORTANT]
> To use [EclipseLink](https://eclipse.dev/eclipselink), you need to ensure that you have done two things:
Copy link
Contributor

Choose a reason for hiding this comment

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

EclipseLink is already linked above, so let's not link it here.

@@ -55,6 +55,13 @@ Be sure to secure your metastore backend since it will be storing credentials an

To use [EclipseLink](https://eclipse.dev/eclipselink/) for metastore management, specify the configuration `metaStoreManager.conf-file` to point to an [EclipseLink `persistence.xml` file](https://eclipse.dev/eclipselink/documentation/2.5/solutions/testingjpa002.htm). This file, local to the Polaris service, will contain information on what database to use for metastore management and how to connect to it.

> [!IMPORTANT]
> To use [EclipseLink](https://eclipse.dev/eclipselink), you need to ensure that you have done two things:
> 1. Build the jar for the EclipseLink extension
Copy link
Contributor

Choose a reason for hiding this comment

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

style: To match the tense of the above line, this should be Built not Build

> 1. Build the jar for the EclipseLink extension
> 2. Set the property on the Polaris project to include eclipseLink.
>
> This can be achieved by setting `eclipseLink=true` in the `gradle.properties` file, or by passing the property explicitly while building all jars, e.g.: `./gradlew -PeclipseLink=true clean assemble`
Copy link
Contributor

Choose a reason for hiding this comment

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

style: JAR is an acronym. It's used above, too.

Copy link

@anuragmantri anuragmantri left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @danielvdende

@@ -55,6 +55,13 @@ Be sure to secure your metastore backend since it will be storing credentials an

To use [EclipseLink](https://eclipse.dev/eclipselink/) for metastore management, specify the configuration `metaStoreManager.conf-file` to point to an [EclipseLink `persistence.xml` file](https://eclipse.dev/eclipselink/documentation/2.5/solutions/testingjpa002.htm). This file, local to the Polaris service, will contain information on what database to use for metastore management and how to connect to it.

> [!IMPORTANT]
> To use EclipseLink, you need to ensure that you have done two things:
Copy link
Member

@RussellSpitzer RussellSpitzer Aug 28, 2024

Choose a reason for hiding this comment

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

A few nits,

Avoid "you" -

EclipseLink requires:
1. Building the jar for the EclipseLink extension
2. Setting the `eclipseLink` gradle property to true within the Polaris build.

Or Something like that

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants