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.

Sign up for GitHub

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

Handle restart cases and add more logging to Eclipselink #1125

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Handle restart cases and add more logging to Eclipselink #1125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion getting-started/assets/eclipselink/persistence.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
<class>org.apache.polaris.jpa.models.ModelEntity</class>
<class>org.apache.polaris.jpa.models.ModelEntityActive</class>
<class>org.apache.polaris.jpa.models.ModelEntityChangeTracking</class>
<class>org.apache.polaris.jpa.models.ModelEntityDropped</class>
<class>org.apache.polaris.jpa.models.ModelGrantRecord</class>
<class>org.apache.polaris.jpa.models.ModelPrincipalSecrets</class>
<class>org.apache.polaris.jpa.models.ModelSequenceId</class>
Expand All @@ -40,6 +39,8 @@
<property name="eclipselink.persistence-context.flush-mode" value="auto"/>
<property name="eclipselink.session.customizer" value="org.apache.polaris.extension.persistence.impl.eclipselink.PolarisEclipseLinkSessionCustomizer"/>
<property name="eclipselink.transaction.join-existing" value="true"/>
<property name="eclipselink.logging.level.sql" value="FINE"/>
<property name="eclipselink.logging.parameters" value="true"/>
</properties>
</persistence-unit>
</persistence>
17 changes: 17 additions & 0 deletions getting-started/eclipselink/docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,27 @@ services:
timeout: 10s
retries: 10

polaris-purge:
# IMPORTANT: the image MUST contain the Postgres JDBC driver and EclipseLink dependencies, see README for instructions
image: apache/polaris-admin-tool:postgres-latest
depends_on:
postgres:
condition: service_started
environment:
polaris.persistence.type: eclipse-link
polaris.persistence.eclipselink.configuration-file: /deployments/config/eclipselink/persistence.xml
volumes:
- ../assets/eclipselink/:/deployments/config/eclipselink
command:
- "purge"
- "--realm=POLARIS"
Comment on lines +64 to +65
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little scary; it's probably fine since this is a getting started image but technically anyone who's been using this may be surprised to find their metastore purged when they update their code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree with you, the current behaviour is it that it, always tries to bootstrap, which fails the whole process if this has been run before, so one is expected to remove this step or run the purge, one other way imho is to see its a clean getting started .

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can have it still fail if you need purge, but we allow you to re-run docker with some simple flag that causes the purge command to run? That way we don't risk someone accidentally purging, but we still improve the frustrating UX you describe.

If not, I am ok to go forward with the PR as written.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe consider use env to control it? If we need to purge, set the env which then set the command to this one. If not set, then fallback default commend (or do nothing).

Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this container, but the issue, as @singhpk234 noted, is that if you shut down the containers, then run them again, the realm will be already bootstrapped and polaris-bootstrap will fail.

We don't have currently a way to say "ignore already-bootstrapped realms" when running the bootstrap command. I was planning to add that, but got side-tracked. That would imho be the best "fix" for the issue here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like @adutra's idea of a flag to ignore already-bootstrapped realms. @singhpk234 do you think that would solve the problem sufficiently?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idea of a flag to ignore already-bootstrapped realms

@eric-maynard I agree, This is a nice suggestion, and could handle this restart issue fairly ! @adutra please let me know if i can help somehow in getting this in.


polaris-bootstrap:
# IMPORTANT: the image MUST contain the Postgres JDBC driver and EclipseLink dependencies, see README for instructions
image: apache/polaris-admin-tool:postgres-latest
depends_on:
polaris-purge:
condition: service_started
postgres:
condition: service_healthy
environment:
Expand Down