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

Jump to bottom

Enable eclipselink with H2 database in testing #158

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Enable eclipselink with H2 database in testing #158

merged 1 commit into from
Sep 11, 2024

Conversation

aihuaxu
Copy link
Contributor

@aihuaxu aihuaxu commented Aug 18, 2024 •
edited
Loading

Description

The change is to support the testing against eclipselink with file-based H2 database in addition to in-memory implementation. That will test the env closer to the production.
There is a difference between in-memory metastore and eclipseLink metastore: in-memory gets bootstrapped automatically since it's only used for test purpose while eclipseLink metastore doesn't boostrap automatically to avoid accidental wipeout in production.

To test out against eclipseLink metastore,

  • The metastore gets bootstrapped for non in-memory metastore in tests (PolarisConnectionExtension).
  • The test DB location is placed in testDir and they will be cleaned up when the test starts to make the tests clean.
  • Remove persistence.xml in test and only keep one persistence.xml in main. I don't see value to keep two since the production version will always be a external version.

TODO:

  • There are some tests currently only testing against in-memory. We will need to add the coverage for elicplseLink.
  • Instead of adding H2 dependencies to polaris-service, we can support loading the additional dependencies ondemand.

Fixes #149

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • This has been tested against with the existing tests ./gradlew test and all the tests passed against eclipselink.
  • This has been tested against with the existing tests ./gradlew test and all the tests passed against in-memory.

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

Please delete options that are not relevant.

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue
  • I have signed and submitted the ICLA and if needed, the CCLA. See Contributing for details.

@aihuaxu aihuaxu requested a review from a team as a code owner August 18, 2024 05:50
@aihuaxu aihuaxu force-pushed the aixu-test-eclipselink-db branch 2 times, most recently from dbc4d23 to 52010b7 Compare August 18, 2024 20:47
collado-mike
collado-mike requested changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@collado-mike collado-mike left a comment

Choose a reason for hiding this comment

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

I think the PolarisConnectionExtension class is the best place to enable bootstrapping the metastore for tests. When we added the bootstrap command, we explicitly removed this logic from the persistent metastore impls so that users couldn't accidentally bootstrap their production instance.

polaris-core/src/main/java/io/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java Outdated Show resolved Hide resolved
@RussellSpitzer
Copy link
Member

@aihuaxu Could you please rebase this?

@aihuaxu
Copy link
Contributor Author

aihuaxu commented Aug 26, 2024

@aihuaxu Could you please rebase this?

@RussellSpitzer Rebased. Please help take a look.

@aihuaxu aihuaxu requested a review from takidau as a code owner August 26, 2024 21:45
eric-maynard
eric-maynard reviewed Aug 27, 2024
View reviewed changes
...e/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java Outdated Show resolved Hide resolved
...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java Outdated Show resolved Hide resolved
polaris-service/build.gradle.kts
@@ -108,6 +108,9 @@ dependencies {
testImplementation(libs.assertj.core)
testImplementation(libs.mockito.core)
testRuntimeOnly("org.junit.platform:junit-platform-launcher")

testRuntimeOnly(project(":polaris-eclipselink"))
Copy link
Contributor

Choose a reason for hiding this comment

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

For now I think this could be okay, but I feel like we may not want these dependencies here forever. We have intentionally not included these dependencies in the service. A new project to include all these dependencies could make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we create a separate project to include the dependencies, we still need to include such project here , right? This is for test only though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to address in the following PRs to include the dependency if needed. I saw that we have -PeclipseLink=$ECLIPSELINK.

...is-service/src/test/java/org/apache/polaris/service/admin/PolarisOverlappingCatalogTest.java Outdated Show resolved Hide resolved
polaris-service/src/test/java/org/apache/polaris/service/test/PolarisConnectionExtension.java Outdated Show resolved Hide resolved
@aihuaxu aihuaxu force-pushed the aixu-test-eclipselink-db branch 2 times, most recently from 17919f5 to a77ca9f Compare August 27, 2024 23:25
flyrain
flyrain reviewed Aug 29, 2024
View reviewed changes
Copy link
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

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

Thanks @aihuaxu for the PR. LGTM overall, left minor comments.

...ervice/src/test/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandlerTest.java Outdated Show resolved Hide resolved
polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java Outdated Show resolved Hide resolved
flyrain
flyrain previously approved these changes Aug 29, 2024
View reviewed changes
@aihuaxu aihuaxu force-pushed the aixu-test-eclipselink-db branch 2 times, most recently from ae5502f to 283710f Compare September 3, 2024 17:34
@sfc-gh-aixu
Copy link
Contributor

@RussellSpitzer, @eric-maynard and @collado-mike Can you help review again?

@flyrain
Copy link
Contributor

flyrain commented Sep 11, 2024

retest this please

flyrain
flyrain approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

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

+1

@flyrain flyrain enabled auto-merge (squash) September 11, 2024 20:38
@flyrain
Copy link
Contributor

flyrain commented Sep 11, 2024

Thanks @aihuaxu for working on it. Thanks @collado-mike @RussellSpitzer @eric-maynard for the review.

@flyrain flyrain merged commit fc6b518 into apache:main Sep 11, 2024
3 checks passed
@aihuaxu
Copy link
Contributor Author

aihuaxu commented Sep 11, 2024

Thanks @flyrain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flyrain flyrain flyrain approved these changes

@dennishuo dennishuo Awaiting requested review from dennishuo

@emkornfield emkornfield Awaiting requested review from emkornfield

@jbonofre jbonofre Awaiting requested review from jbonofre jbonofre is a code owner

@ashvina ashvina Awaiting requested review from ashvina ashvina is a code owner

@RussellSpitzer RussellSpitzer Awaiting requested review from RussellSpitzer RussellSpitzer is a code owner

@snazy snazy Awaiting requested review from snazy snazy is a code owner

@vvcephei vvcephei Awaiting requested review from vvcephei vvcephei is a code owner

@jackye1995 jackye1995 Awaiting requested review from jackye1995 jackye1995 is a code owner

@collado-mike collado-mike Awaiting requested review from collado-mike collado-mike is a code owner

@takidau takidau Awaiting requested review from takidau takidau is a code owner

@eric-maynard eric-maynard Awaiting requested review from eric-maynard eric-maynard is a code owner

@sfc-gh-aixu sfc-gh-aixu Awaiting requested review from sfc-gh-aixu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Switch the tests against EclipseLink with H2 database
6 participants
@aihuaxu @RussellSpitzer @sfc-gh-aixu @flyrain @eric-maynard @collado-mike