eric-maynard commented on code in PR #158:
URL: https://github.com/apache/polaris/pull/158#discussion_r1733357529
##########
extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/eclipselink/PolarisEclipseLinkMetaStoreSessionImpl.java:
##########
@@ -153,8 +154,8 @@ private EntityManagerFactory createEntityManagerFactory(
prefixUrl = new File(jarPrefixPath).toURI().toURL();
}
- LOGGER.info(
- "Created a new ClassLoader with the jar {} in classpath to load
the config file",
+ LOGGER.debug(
+ "Create a new ClassLoader with the jar {} in classpath to load the
config file",
Review Comment:
ultra nit: `Creating`?
##########
polaris-service/src/test/java/org/apache/polaris/service/test/PolarisConnectionExtension.java:
##########
@@ -109,10 +118,40 @@ public void beforeAll(ExtensionContext extensionContext)
throws Exception {
}
}
+ @Override
+ public void afterAll(ExtensionContext context) {
+ if (metaStoreManagerFactory instanceof
LocalPolarisMetaStoreManagerFactory) {
+ metaStoreManagerFactory.purgeRealms(List.of(realm));
+ }
+ }
+
public static String getTestRealm(Class testClassName) {
return testClassName.getName().replace('.', '_');
}
+ public static void createTestDir(String realm) throws IOException {
+ // Set up the database location
+ Path testDir = Path.of("build/test_data/iceberg/" + realm);
Review Comment:
iceberg?
##########
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"))
Review Comment:
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.
##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisOverlappingCatalogTest.java:
##########
@@ -110,101 +119,72 @@ private Response createCatalog(
}
}
- @Test
- public void testBasicOverlappingCatalogs() {
- Arrays.asList(false, true)
- .forEach(
- initiallyExternal -> {
- Arrays.asList(false, true)
- .forEach(
- laterExternal -> {
- String prefix = UUID.randomUUID().toString();
-
- assertThat(createCatalog(prefix, "root",
initiallyExternal))
- .returns(Response.Status.CREATED.getStatusCode(),
Response::getStatus);
-
- // OK, non-overlapping
- assertThat(createCatalog(prefix, "boot",
laterExternal))
- .returns(Response.Status.CREATED.getStatusCode(),
Response::getStatus);
-
- // OK, non-overlapping due to no `/`
- assertThat(createCatalog(prefix, "roo", laterExternal))
- .returns(Response.Status.CREATED.getStatusCode(),
Response::getStatus);
-
- // Also OK due to no `/`
- assertThat(createCatalog(prefix, "root.child",
laterExternal))
- .returns(Response.Status.CREATED.getStatusCode(),
Response::getStatus);
-
- // inside `root`
- assertThat(createCatalog(prefix, "root/child",
laterExternal))
- .returns(
- Response.Status.BAD_REQUEST.getStatusCode(),
Response::getStatus);
-
- // `root` is inside this
- assertThat(createCatalog(prefix, "", laterExternal))
- .returns(
- Response.Status.BAD_REQUEST.getStatusCode(),
Response::getStatus);
- });
- });
+ @ParameterizedTest
Review Comment:
Are these changes related to the PR?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java:
##########
@@ -46,7 +46,6 @@
*/
public abstract class LocalPolarisMetaStoreManagerFactory<StoreType>
implements MetaStoreManagerFactory {
-
Review Comment:
nit: spurious change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
