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

[BUG] Potential eclipselink schema upgrade issue #450

Open
1 task done
snazy opened this issue Nov 15, 2024 · 8 comments · May be fixed by #456 or #461
Open
1 task done

[BUG] Potential eclipselink schema upgrade issue #450

snazy opened this issue Nov 15, 2024 · 8 comments · May be fixed by #456 or #461
Labels
bug Something isn't working

Comments

@snazy
Copy link
Member

snazy commented Nov 15, 2024

Is this a possible security vulnerability?

  • This is NOT a possible security vulnerability

Describe the bug

The change #438 changes the database schema. However, there seems to be an issue that the schema is not properly updated - at least not via PolarisEclipseLinkMetaStoreManagerTest, see repro steps below.

/cc @eric-maynard

To Reproduce

  1. git checkout e232b000760da335721e9112f9d52fe041289613 (commit before Do not persist plaintext secrets in the metastore #438)
  2. rm -rf extension/persistence/eclipselink/build (just get a clean state)
  3. ./gradlew :polaris-eclipselink:test --rerun (passes fine)
  4. git checkout ab095cfab84e5b4e694c72011f4166589eb25f38 (Do not persist plaintext secrets in the metastore #438)
  5. ./gradlew :polaris-eclipselink:test --rerun (fails)
  6. rm -rf extension/persistence/eclipselink/build/test_data (database state)
  7. ./gradlew :polaris-eclipselink:test --rerun (passes fine)

Actual Behavior

No response

Expected Behavior

No response

Additional context

No response

System information

No response

@eric-maynard
Copy link
Contributor

eric-maynard commented Nov 18, 2024

I'm able to repro this using the steps provided.

I believe that the simplest fix is to enable automatic schema migrations in eclipselink by setting:

<property name="eclipselink.ddl-generation" value="create-or-extend-tables"/>

By adding this between steps (4) and (5) in your instructions, I am able to get the tests in step (5) to mostly pass -- PolarisEclipseLinkMetaStoreManagerTest.testPrivileges still fails, but I think that's a different issue.

I've noted this down this on the PR that caused this, as well.

@eric-maynard
Copy link
Contributor

For the time being, shall we revert the offending PR?

I am also happy to fix-forward with the config suggested above. In any case, I think this is a good indicator that cross-version tests would be a good idea.

@snazy
Copy link
Member Author

snazy commented Nov 18, 2024

Honestly, I don't think that reverting is an option.

Cross-version tests (or backwards/forwards/migration compatibility tests) are a good idea!
SQL schema migration however complicates things, especially in a distributed / horizontally scalable system w/ rolling upgrades.

Re cross-version tests (backwards/forwards compatibility + rolling upgrades), we do have something like that in Nessie, where we run REST services from older releases and test the newest client (on main) against it, and older clients against the newest REST service (on main). It requires quite a bit of class-loading trickery though. With Iceberg/Java (which we don't use in those tests in Nessie) it would be quite hard, because Iceberg/Java and especially the Azure SDK keep resources like threads and thread-locals around, which cause class leaks that then cause OOMs - not great.

However, testing relevant things (persistence is one) in isolation should be doable - as dedicated / isolated tests.

@eric-maynard eric-maynard linked a pull request Nov 18, 2024 that will close this issue
5 tasks
@eric-maynard
Copy link
Contributor

eric-maynard commented Nov 18, 2024

I see. Yeah, I think it will be complicated to set up true cross-version tests. However since we do have a persistence layer I think it will be important to ensure that upgrades/downgrades can work with various states in the persistence layer. Other stuff, like cross-version notification compatibility, could come later.

Filed this PR to fix-forward: #456

I want to look more closely at the one test that's failing; I think that's a legitimate issue with #438. Perhaps the ID sequence does not get preserved when the schema is automatically upgraded.

@collado-mike
Copy link
Contributor

Honestly, I don't think that reverting is an option.

Why is this not an option?

@eric-maynard
Copy link
Contributor

cc @jbonofre -- with the release coming up, how do you want to approach this?

Currently, anyone who was using EclipseLink-backed Polaris prior to #438 (who has not configured EclipseLink for automatic schema migrations) will not be able to upgrade to 0.9.0 without a manual schema migration. On the other hand, if we revert, anyone who created a metastore between #438 merging and being reverted will be stuck.

@adutra
Copy link
Contributor

adutra commented Nov 19, 2024

anyone who was using EclipseLink-backed Polaris prior to #438 (who has not configured EclipseLink for automatic schema migrations) will not be able to upgrade to 0.9.0 without a manual schema migration

If they haven't set up schema migration, that's expected right? Couldn't we just mention this fact in the release notes and keep #438 in 0.9.0?

@eric-maynard
Copy link
Contributor

eric-maynard commented Nov 19, 2024

@adutra that would be reasonable, too. I guess the question is whether we want to force users to do schema migrations (maybe only across minor/major versions?) or whether we want this to be something Polaris itself handles.

In either case I think it should be an explicit decision. If we want users to do this manually, we should not merge #456

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants