Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.polaris.extension.persistence.impl.eclipselink;

import io.dropwizard.jackson.Discoverable;
import io.dropwizard.jackson.DiscoverableSubtypeResolver;

/**
* Discoverability aid for Dropwizard. Leaf classes cannot implement {@link Discoverable} directly,
* they have to implement another interface, which implements {@link Discoverable}.
*
* @see DiscoverableSubtypeResolver
*/
public interface DiscoverableMetaStoreManagerFactory extends Discoverable {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make a lot of sense to me. When the subtype resolver is asked for implementations, it'll be for a common interface - i.e., MetaStoreManagerFactory in this case. Why would the subtype resolver ever ask for the eclipse-link specific version of the DiscoverableMetaStoreManagerFactory?

Copy link
Contributor Author

@dimas-b dimas-b Nov 14, 2024

Choose a reason for hiding this comment

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

Type resolution during YAML parsing is handled by Jackson, and it properly recognizes all types in the java hierarchy (even those not implementing Discoverable).

However, Jackson (in this setup) relies on DiscoverableSubtypeResolver to discover implementation classes. That latter part has peculiar logic, where DiscoverableSubtypeResolver performs a two stage lookup: 1) classes listed in META-INF/services/io.dropwizard.jackson.Discoverable, 2) classes listed in the service manifests for the classes from step 1. I do not really know why it does not directly inject classes from step 1 into Jackson.

I agree, it looks odd. However, having one Discoverable interface is not possible as this PR specifically proposes to remove DW dependencies from polaris-core, where the base interface exists and there is no other common root between the "in memory" metastore implementation and the EclipseLink implementation. We could add a rather small common module for that, of course, but in my mind having two separate Discoverable interfaces was less intrusive. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Leaf classes cannot implement {@link Discoverable} directly, they have to implement another interface, which implements {@link Discoverable}.

Isn't this the exact setup we have right now, where the leaf classes implement MetastoreManagerFactory?

Should we just move MetastoreManagerFactory out of core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we just move MetastoreManagerFactory out of core?

That's an option. This is what I meant by "add a rather small common module" above, because the EclipseLink and In-Memory implementations have to share it.

Would do other people think? @collado-mike @adutra ?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, if I get this correctly, we would all love to have MetastoreManagerFactory directly in polaris-service like many other similar beans, but we can't, because of EclipseLink.

If that is correct, I'd prefer to keep your current solution for now, then revisit this later, when/if EclipseLink is replaced with some more robust persistence solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

(and in any case, I'd avoid introducing a "polaris-core-dropwizard" module just for that.)

Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@
*/
@JsonTypeName("eclipse-link")
public class EclipseLinkPolarisMetaStoreManagerFactory
extends LocalPolarisMetaStoreManagerFactory<PolarisEclipseLinkStore> {
extends LocalPolarisMetaStoreManagerFactory<PolarisEclipseLinkStore>
implements DiscoverableMetaStoreManagerFactory {
@JsonProperty("conf-file")
private String confFile;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
#

org.apache.polaris.extension.persistence.impl.eclipselink.DiscoverableMetaStoreManagerFactory
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,4 @@
# under the License.
#

org.apache.polaris.service.persistence.InMemoryPolarisMetaStoreManagerFactory
org.apache.polaris.extension.persistence.impl.eclipselink.EclipseLinkPolarisMetaStoreManagerFactory
4 changes: 0 additions & 4 deletions polaris-core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@ dependencies {
constraints {
implementation("io.airlift:aircompressor:0.27") { because("Vulnerability detected in 0.25") }
}
// TODO - this is only here for the Discoverable interface
// We should use a different mechanism to discover the plugin implementations
implementation(platform(libs.dropwizard.bom))
implementation("io.dropwizard:dropwizard-jackson")

implementation(platform(libs.jackson.bom))
implementation("com.fasterxml.jackson.core:jackson-annotations")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.apache.polaris.core.persistence;

import com.fasterxml.jackson.annotation.JsonTypeInfo;
import io.dropwizard.jackson.Discoverable;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;
Expand All @@ -34,7 +33,7 @@
* configuration
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type")
public interface MetaStoreManagerFactory extends Discoverable {
public interface MetaStoreManagerFactory {

PolarisMetaStoreManager getOrCreateMetaStoreManager(RealmContext realmContext);

Expand Down
2 changes: 1 addition & 1 deletion polaris-service/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ dependencies {
testImplementation(libs.mockito.core)
testRuntimeOnly("org.junit.platform:junit-platform-launcher")

testRuntimeOnly(project(":polaris-eclipselink"))
testImplementation(project(":polaris-eclipselink"))
}

if (project.properties.get("eclipseLink") == "true") {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.polaris.service.persistence;

import io.dropwizard.jackson.Discoverable;
import io.dropwizard.jackson.DiscoverableSubtypeResolver;

/**
* Discoverability aid for Dropwizard. Leaf classes cannot implement {@link Discoverable} directly,
* they have to implement another interface, which implements {@link Discoverable}.
*
* @see DiscoverableSubtypeResolver
*/
public interface DiscoverableMetaStoreManagerFactory extends Discoverable {}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@

@JsonTypeName("in-memory")
public class InMemoryPolarisMetaStoreManagerFactory
extends LocalPolarisMetaStoreManagerFactory<PolarisTreeMapStore> {
extends LocalPolarisMetaStoreManagerFactory<PolarisTreeMapStore>
implements DiscoverableMetaStoreManagerFactory {
final Set<String> bootstrappedRealms = new HashSet<>();

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#

org.apache.polaris.service.auth.DiscoverableAuthenticator
org.apache.polaris.core.persistence.MetaStoreManagerFactory
org.apache.polaris.service.persistence.DiscoverableMetaStoreManagerFactory
org.apache.polaris.service.config.OAuth2ApiService
org.apache.polaris.service.context.RealmContextResolver
org.apache.polaris.service.context.CallContextResolver
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
#

org.apache.polaris.service.persistence.InMemoryPolarisMetaStoreManagerFactory
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.polaris.service;

import static org.assertj.core.api.Assertions.assertThat;

import io.dropwizard.testing.ConfigOverride;
import io.dropwizard.testing.ResourceHelpers;
import io.dropwizard.testing.junit5.DropwizardAppExtension;
import io.dropwizard.testing.junit5.DropwizardExtensionsSupport;
import org.apache.polaris.extension.persistence.impl.eclipselink.EclipseLinkPolarisMetaStoreManagerFactory;
import org.apache.polaris.service.config.PolarisApplicationConfig;
import org.apache.polaris.service.persistence.InMemoryPolarisMetaStoreManagerFactory;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

@ExtendWith(DropwizardExtensionsSupport.class)
public class PolarisApplicationConfigurationTest {

public static final String CONFIG_PATH =
ResourceHelpers.resourceFilePath("polaris-server-integrationtest.yml");
// Bind to random ports to support parallelism
public static final ConfigOverride RANDOM_APP_PORT =
ConfigOverride.config("server.applicationConnectors[0].port", "0");
public static final ConfigOverride RANDOM_ADMIN_PORT =
ConfigOverride.config("server.adminConnectors[0].port", "0");

@Nested
class DefaultMetastore {
private final DropwizardAppExtension<PolarisApplicationConfig> app =
new DropwizardAppExtension<>(
PolarisApplication.class, CONFIG_PATH, RANDOM_APP_PORT, RANDOM_ADMIN_PORT);

@Test
void testMetastoreType() {
assertThat(app.getConfiguration().getMetaStoreManagerFactory())
.isInstanceOf(InMemoryPolarisMetaStoreManagerFactory.class);
}
}

@Nested
class EclipseLinkMetastore {
private final DropwizardAppExtension<PolarisApplicationConfig> app =
new DropwizardAppExtension<>(
PolarisApplication.class,
CONFIG_PATH,
RANDOM_APP_PORT,
RANDOM_ADMIN_PORT,
ConfigOverride.config("metaStoreManager.type", "eclipse-link"),
ConfigOverride.config("metaStoreManager.persistence-unit", "test-unit"),
ConfigOverride.config("metaStoreManager.conf-file", "/test-conf-file"));

@Test
void testMetastoreType() {
assertThat(app.getConfiguration().getMetaStoreManagerFactory())
.isInstanceOf(EclipseLinkPolarisMetaStoreManagerFactory.class)
.extracting("persistenceUnitName", "confFile")
.containsExactly("test-unit", "/test-conf-file");
}
}
}