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

Persistence Refactor POC #1011

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Persistence Refactor POC #1011

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da98100
Persistence Refactor POC
Feb 17, 2025
1bbe19f
Retrigger the pipeline
Feb 17, 2025
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
21 changes: 21 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/CatalogDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* 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.core.persistence.dao;

public interface CatalogDao {}
Copy link
Member

@jbonofre jbonofre Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For persistence DAO, my idea was more to have concrete DAO definition to help the implementer with associated record. The DAO deals with storage operation using a record carriage (to avoid coupliing with entity).

Something like:

Suggested change
public interface CatalogDao {}
public record CatalogRecord(String id, String name, String location, ...) {
...
}

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'm not sure about this. We should refactor the existing service entities instead of creating a new set of them as I said in the mailing list.

Copy link
Member

Choose a reason for hiding this comment

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

@flyrain you are right for DAO, my point is more for using entity in the DAO. I would rather use a record in the DAO to decouple from Entity: it will force us to decouple service and storage layers, and the record should be "obvious" for DAO implementer.

37 changes: 37 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/NamespaceDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* 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.core.persistence.dao;

import java.util.List;
import org.apache.iceberg.catalog.Namespace;
import org.apache.polaris.core.entity.NamespaceEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityCore;

public interface NamespaceDao {
void save(NamespaceEntity namespace, List<PolarisEntityCore> catalogPath);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to have operation definitions in the DAO. The DAO could just a record (public record NamespaceDao), the storage logic for a DAO is in the persistence implementation.

The idea of DAO is to "map" business logic object to persistence object, I think it's a paradigm we should keep here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea of DAO is to "map" business logic object to persistence object

To be clear, NamespaceEntity and catalogPath here should service-layer entities. I put them here to demo how it works with existing code. But they are not necessary to be the final form. The idea of the DAO is to provide an interface so that the impl. can do two-way conversion like the following, while keep the interface only with upper-layer entities.

  1. service entities -> persistence entities
  2. persistence entities -> service entities

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it makes sense. As said in previous comment, the DAO as storage operation definition can do the mapping but I think he should use only storage records (persistence entities) that can be converted to service entitiy. Seperation here is welcome.


void update(NamespaceEntity namespace, List<PolarisEntityCore> catalogPath);

NamespaceEntity get(String namespaceId);

NamespaceEntity get(Namespace namespace);

boolean delete(List<PolarisEntityCore> catalogPath, PolarisEntity leafEntity, boolean cleanup);
}
15 changes: 11 additions & 4 deletions .../service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisAuthzTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import org.apache.polaris.core.persistence.PolarisEntityManager;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.apache.polaris.core.persistence.dao.NamespaceDao;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
import org.apache.polaris.service.admin.PolarisAdminService;
import org.apache.polaris.service.catalog.BasePolarisCatalog;
Expand All @@ -77,6 +78,7 @@
import org.apache.polaris.service.config.DefaultConfigurationStore;
import org.apache.polaris.service.config.RealmEntityManagerFactory;
import org.apache.polaris.service.context.PolarisCallContextCatalogFactory;
import org.apache.polaris.service.persistence.fdb.dao.FdbNamespaceDao;
import org.apache.polaris.service.storage.PolarisStorageIntegrationProviderImpl;
import org.apache.polaris.service.task.TaskExecutor;
import org.assertj.core.api.Assertions;
Expand Down Expand Up @@ -177,6 +179,7 @@ public Map<String, String> getConfigOverrides() {
protected PrincipalEntity principalEntity;
protected RealmContext realmContext;
protected AuthenticatedPolarisPrincipal authenticatedRoot;
protected NamespaceDao namespaceDao;

@BeforeAll
public static void setUpMocks() {
Expand All @@ -194,6 +197,7 @@ public void before(TestInfo testInfo) {
metaStoreManager = managerFactory.getOrCreateMetaStoreManager(realmContext);
metaStoreSession = managerFactory.getOrCreateSessionSupplier(realmContext).get();
entityManager = realmEntityManagerFactory.getOrCreateEntityManager(realmContext);
namespaceDao = new FdbNamespaceDao(metaStoreManager, metaStoreSession);

PrincipalEntity rootEntity =
new PrincipalEntity(
Expand Down Expand Up @@ -409,7 +413,8 @@ private void initBaseCatalog() {
passthroughView,
securityContext,
Mockito.mock(),
fileIOFactory);
fileIOFactory,
namespaceDao);
this.baseCatalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
Expand All @@ -422,7 +427,7 @@ public static class TestPolarisCallContextCatalogFactory
extends PolarisCallContextCatalogFactory {

public TestPolarisCallContextCatalogFactory() {
super(null, null, null, null, null, null, null);
super(null, null, null, null, null, null, null, null);
}

@Inject
Expand All @@ -433,15 +438,17 @@ public TestPolarisCallContextCatalogFactory(
PolarisConfigurationStore configurationStore,
PolarisDiagnostics diagnostics,
TaskExecutor taskExecutor,
FileIOFactory fileIOFactory) {
FileIOFactory fileIOFactory,
NamespaceDao namespaceDao) {
super(
entityManager,
metaStoreManager,
metaStoreSession,
configurationStore,
diagnostics,
taskExecutor,
fileIOFactory);
fileIOFactory,
namespaceDao);
}

@Override
Expand Down
22 changes: 16 additions & 6 deletions ...vice/src/test/java/org/apache/polaris/service/quarkus/catalog/BasePolarisCatalogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet;
import org.apache.polaris.core.persistence.cache.EntityCache;
import org.apache.polaris.core.persistence.dao.NamespaceDao;
import org.apache.polaris.core.storage.PolarisCredentialProperty;
import org.apache.polaris.core.storage.PolarisStorageIntegration;
import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;
Expand All @@ -101,6 +102,7 @@
import org.apache.polaris.service.catalog.io.MeasuredFileIOFactory;
import org.apache.polaris.service.config.RealmEntityManagerFactory;
import org.apache.polaris.service.exception.IcebergExceptionMapper;
import org.apache.polaris.service.persistence.fdb.dao.FdbNamespaceDao;
import org.apache.polaris.service.storage.PolarisStorageIntegrationProviderImpl;
import org.apache.polaris.service.task.TableCleanupTaskHandler;
import org.apache.polaris.service.task.TaskExecutor;
Expand Down Expand Up @@ -161,6 +163,7 @@ public Map<String, String> getConfigOverrides() {
private RealmContext realmContext;
private PolarisMetaStoreManager metaStoreManager;
private PolarisMetaStoreSession metaStoreSession;
private NamespaceDao namespaceDao;
private PolarisAdminService adminService;
private PolarisEntityManager entityManager;
private FileIOFactory fileIOFactory;
Expand All @@ -186,6 +189,7 @@ public void before(TestInfo testInfo) {
realmContext = () -> realmName;
metaStoreManager = managerFactory.getOrCreateMetaStoreManager(realmContext);
metaStoreSession = managerFactory.getOrCreateSessionSupplier(realmContext).get();
namespaceDao = new FdbNamespaceDao(metaStoreManager, metaStoreSession);
entityManager = entityManagerFactory.getOrCreateEntityManager(realmContext);

PrincipalEntity rootEntity =
Expand Down Expand Up @@ -273,7 +277,8 @@ public void before(TestInfo testInfo) {
passthroughView,
securityContext,
taskExecutor,
fileIOFactory);
fileIOFactory,
namespaceDao);
this.catalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
Expand Down Expand Up @@ -528,7 +533,8 @@ public void testValidateNotificationFailToCreateFileIO() throws IOException {
passthroughView,
securityContext,
Mockito.mock(),
fileIoFactory);
fileIoFactory,
namespaceDao);
catalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
Expand Down Expand Up @@ -856,7 +862,8 @@ public void testUpdateNotificationCreateTableWithLocalFilePrefix() {
passthroughView,
securityContext,
taskExecutor,
fileIOFactory);
fileIOFactory,
namespaceDao);
catalog.initialize(
catalogWithoutStorage,
ImmutableMap.of(
Expand Down Expand Up @@ -921,7 +928,8 @@ public void testUpdateNotificationCreateTableWithHttpPrefix() {
passthroughView,
securityContext,
taskExecutor,
fileIOFactory);
fileIOFactory,
namespaceDao);
catalog.initialize(
catalogName,
ImmutableMap.of(
Expand Down Expand Up @@ -1454,7 +1462,8 @@ public void testDropTableWithPurgeDisabled() {
passthroughView,
securityContext,
Mockito.mock(),
fileIOFactory);
fileIOFactory,
namespaceDao);
noPurgeCatalog.initialize(
noPurgeCatalogName,
ImmutableMap.of(
Expand Down Expand Up @@ -1539,7 +1548,8 @@ public void testFileIOWrapper() {
passthroughView,
securityContext,
Mockito.mock(),
measured);
measured,
namespaceDao);
catalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
Expand Down
7 changes: 6 additions & 1 deletion .../src/test/java/org/apache/polaris/service/quarkus/catalog/BasePolarisCatalogViewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,14 @@
import org.apache.polaris.core.persistence.PolarisEntityManager;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.apache.polaris.core.persistence.dao.NamespaceDao;
import org.apache.polaris.service.admin.PolarisAdminService;
import org.apache.polaris.service.catalog.BasePolarisCatalog;
import org.apache.polaris.service.catalog.PolarisPassthroughResolutionView;
import org.apache.polaris.service.catalog.io.DefaultFileIOFactory;
import org.apache.polaris.service.catalog.io.FileIOFactory;
import org.apache.polaris.service.config.RealmEntityManagerFactory;
import org.apache.polaris.service.persistence.fdb.dao.FdbNamespaceDao;
import org.apache.polaris.service.storage.PolarisStorageIntegrationProviderImpl;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
Expand Down Expand Up @@ -101,6 +103,7 @@ public Map<String, String> getConfigOverrides() {

private PolarisMetaStoreManager metaStoreManager;
private PolarisMetaStoreSession metaStoreSession;
private NamespaceDao namespaceDao;

@BeforeAll
public static void setUpMocks() {
Expand All @@ -127,6 +130,7 @@ public void before(TestInfo testInfo) {

metaStoreManager = managerFactory.getOrCreateMetaStoreManager(realmContext);
metaStoreSession = managerFactory.getOrCreateSessionSupplier(realmContext).get();
namespaceDao = new FdbNamespaceDao(metaStoreManager, metaStoreSession);

PrincipalEntity rootEntity =
new PrincipalEntity(
Expand Down Expand Up @@ -187,7 +191,8 @@ public void before(TestInfo testInfo) {
passthroughView,
securityContext,
Mockito.mock(),
fileIOFactory);
fileIOFactory,
namespaceDao);
this.catalog.initialize(
CATALOG_NAME,
ImmutableMap.of(
Expand Down
7 changes: 4 additions & 3 deletions ...ava/org/apache/polaris/service/quarkus/catalog/PolarisCatalogHandlerWrapperAuthzTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ private CallContextCatalogFactory newCatalogFactory() {
configurationStore,
diagServices,
Mockito.mock(),
fileIOFactory);
fileIOFactory,
namespaceDao);
}

/**
Expand Down Expand Up @@ -1711,8 +1712,8 @@ public void testSendNotificationSufficientPrivileges() {
configurationStore,
diagServices,
Mockito.mock(),
new DefaultFileIOFactory(
realmEntityManagerFactory, managerFactory, configurationStore)) {
new DefaultFileIOFactory(realmEntityManagerFactory, managerFactory, configurationStore),
namespaceDao) {
@Override
public Catalog createCallContextCatalog(
RealmContext realmContext,
Expand Down
52 changes: 15 additions & 37 deletions service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
import org.apache.polaris.core.persistence.ResolvedPolarisEntity;
import org.apache.polaris.core.persistence.dao.NamespaceDao;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifestCatalogView;
import org.apache.polaris.core.persistence.resolver.ResolverPath;
Expand Down Expand Up @@ -165,6 +166,7 @@ public class BasePolarisCatalog extends BaseMetastoreViewCatalog
private final FileIOFactory fileIOFactory;
private final long catalogId;
private final String catalogName;
private final NamespaceDao namespaceDao;

private String ioImplClassName;
private FileIO catalogFileIO;
Expand All @@ -191,7 +193,8 @@ public BasePolarisCatalog(
PolarisResolutionManifestCatalogView resolvedEntityView,
SecurityContext securityContext,
TaskExecutor taskExecutor,
FileIOFactory fileIOFactory) {
FileIOFactory fileIOFactory,
NamespaceDao namespaceDao) {
this.realmContext = realmContext;
this.entityManager = entityManager;
this.metaStoreManager = metaStoreManager;
Expand All @@ -208,6 +211,7 @@ public BasePolarisCatalog(
(AuthenticatedPolarisPrincipal) securityContext.getUserPrincipal();
this.catalogId = catalogEntity.getId();
this.catalogName = catalogEntity.getName();
this.namespaceDao = namespaceDao;
}

@Override
Expand Down Expand Up @@ -523,17 +527,8 @@ private void createNamespaceInternal(
} else {
LOGGER.debug("Skipping location overlap validation for namespace '{}'", namespace);
}
PolarisEntity returnedEntity =
PolarisEntity.of(
getMetaStoreManager()
.createEntityIfNotExists(
metaStoreSession,
PolarisEntity.toCoreList(resolvedParent.getRawFullPath()),
entity));
if (returnedEntity == null) {
throw new AlreadyExistsException(
"Cannot create namespace %s. Namespace already exists", namespace);
}

namespaceDao.save(entity, PolarisEntity.toCoreList(resolvedParent.getRawFullPath()));
}

private String resolveNamespaceLocation(Namespace namespace, Map<String, String> properties) {
Expand Down Expand Up @@ -633,22 +628,15 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
PolarisEntity leafEntity = resolvedEntities.getRawLeafEntity();

// drop if exists and is empty
PolarisMetaStoreManager.DropEntityResult dropEntityResult =
getMetaStoreManager()
.dropEntityIfExists(
metaStoreSession,
PolarisEntity.toCoreList(catalogPath),
leafEntity,
Map.of(),
configurationStore.getConfiguration(
realmContext, PolarisConfiguration.CLEANUP_ON_NAMESPACE_DROP));

if (!dropEntityResult.isSuccess() && dropEntityResult.failedBecauseNotEmpty()) {
try {
return namespaceDao.delete(
PolarisEntity.toCoreList(catalogPath),
leafEntity,
configurationStore.getConfiguration(
realmContext, PolarisConfiguration.CLEANUP_ON_NAMESPACE_DROP));
} catch (NamespaceNotEmptyException e) {
throw new NamespaceNotEmptyException("Namespace %s is not empty", namespace);
}

// return status of drop operation
return dropEntityResult.isSuccess();
}

@Override
Expand Down Expand Up @@ -678,17 +666,7 @@ public boolean setProperties(Namespace namespace, Map<String, String> properties
}

List<PolarisEntity> parentPath = resolvedEntities.getRawFullPath();
PolarisEntity returnedEntity =
Optional.ofNullable(
getMetaStoreManager()
.updateEntityPropertiesIfNotChanged(
metaStoreSession, PolarisEntity.toCoreList(parentPath), updatedEntity)
.getEntity())
.map(PolarisEntity::new)
.orElse(null);
if (returnedEntity == null) {
throw new RuntimeException("Concurrent modification of namespace: " + namespace);
}
namespaceDao.update(NamespaceEntity.of(updatedEntity), PolarisEntity.toCoreList(parentPath));
return true;
}

Expand Down
Loading
Loading