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

Handle restart cases and add more logging to Eclipselink #1125

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

Conversation

singhpk234
Copy link
Collaborator

@singhpk234 singhpk234 commented Mar 5, 2025

About the change

  1. Add the ecpliselink getting started to capture SQL queries in server logs to enable debugging
  2. Fix the case when the docker images is restarted again by purging the realm first before starting
    a/ we can do alterantive as well before stopping purge the realm.

Comment on lines +64 to +65
- "purge"
- "--realm=POLARIS"
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.

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