Separate Polaris Entities #1128

flyrain · 2025-03-06T08:08:54Z

No description provided.

flyrain · 2025-03-10T17:46:58Z

...src/main/java/org/apache/polaris/extension/persistence/impl/jdbc/PostgresCatalogDaoImpl.java

+import org.apache.polaris.core.persistence.dao.entity.ListEntitiesResult;
+import org.apache.polaris.core.persistence.dao.entity.ResolvedEntityResult;
+
+public class PostgresCatalogDaoImpl implements CatalogDao {


All classes under this package are placeholders for jdbc impl.

imho, then this should be outside the eclipselink package ?

extension/persistence/eclipselink/src/main/java/org/apache/polaris/extension/persistence/impl/jdbc/PostgresCatalogDaoImpl.java

may be extension/persistence ?

extension

persistence

relational

jdbc

eclipselink

yes, we should put them in a different module ideally. I put them here mainly for demo purpose. Let me remove it in the next commit.

flyrain · 2025-03-10T17:48:36Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/CatalogDao.java

+
+  // TODO this should return a type-specific entity result, e.g., CatalogEntityResult
+  @Nonnull
+  EntityResult readEntityByName(@Nonnull PolarisCallContext callCtx, @Nonnull String name);


As I put in the comment and dev mail list, the type-specific return entity refactor will come later.

eric-maynard · 2025-03-10T23:13:53Z

polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/TaskDao.java

Instead of Fdb, I think we need multiple impls:

jdbc

eclipse-link

treemap

"delegating" to the metastoremanager, which is needed temporarily for backwards compatibility

Renamed the current DAO implementations to DelegatingXXXDaoImpl for backward compatibility. We can add JDBC implementation in a followup PR.

Nice, it looks good now. One other note is that the package name transactional is a little confusing but that can wait until later refactors.

flyrain · 2025-03-11T04:47:33Z

service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

  private final AuthenticatedPolarisPrincipal authenticatedPrincipal;
  private final PolarisAuthorizer authorizer;
-  private final PolarisMetaStoreManager metaStoreManager;
+  private final PolarisDaoManager daoManager;


Using this class to show case how business logic invokes type-specific DAO objects. We can easily extend it to other classes like BasePolarisCatalog, PolarisEntityManager and Resolver

eric-maynard

LGTM now as a first step; we'll see the benefits of having the DAOs once we are able to provide metastore-specific DAO implementations and replace direct usage of PolarisMetastoreManager with the DAOs.

snazy

I have strong concerns with the state of this PR.

Generally, I'm strongly +1 on having concern-oriented APIs and concrete implementation of those - for example an API for catalog management, for catalog content, etc. However, IMHO that should really be persistence implementation independent.

While this PR goes into the direction of concern-based APIs, it still exposes persistence internals and pushes those things up to the call sites, which should not be the case.

Another topic is the tight coupling of object-storage specific concerns with Polaris's backend database. That should really be decoupled, because credential vending has nothing to do with Polaris's backend database.

flyrain · 2025-03-11T19:18:25Z

While this PR goes into the direction of concern-based APIs, it still exposes persistence internals and pushes those things up to the call sites, which should not be the case.

Can you point out in the code where "exposes persistence internals and pushes those things up to the call sites"? I'm also open for suggestion how we can separate concern better.

snazy · 2025-03-17T14:52:33Z

Can you point out in the code where "exposes persistence internals and pushes those things up to the call sites"? I'm also open for suggestion how we can separate concern better.

Sure, as mentioned elsewhere, there should eventually be distinct APIs for each concern. Those APIs do not have to and therefore must not deal for example with persistence-internal IDs or dictate how objects are stored but provide the "concern specific" operations.

As that's a more general approach, I really prefer to move this one into draft state and discuss the approach on the dev-ML.

eric-maynard · 2025-03-17T15:43:37Z

Isn’t there a thread from 3/6 already?

…

On Mon, Mar 17, 2025 at 7:52 AM Robert Stupp ***@***.***> wrote: Can you point out in the code where "exposes persistence internals and pushes those things up to the call sites"? I'm also open for suggestion how we can separate concern better. Sure, as mentioned elsewhere, there should eventually be distinct APIs for each concern. Those APIs do not have to and therefore must not deal for example with persistence-internal IDs or dictate how objects are stored but provide the "concern specific" operations. As that's a more general approach, I really prefer to move this one into draft state and discuss the approach on the dev-ML. — Reply to this email directly, view it on GitHub <#1128 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFRE3SA3HOD77KDTYF7RGHL2U3OUTAVCNFSM6AAAAABYOAQGR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMRZHAYTOOBVGY> . You are receiving this because your review was requested.Message ID: ***@***.***> [image: snazy]*snazy* left a comment (apache/polaris#1128) <#1128 (comment)> Can you point out in the code where "exposes persistence internals and pushes those things up to the call sites"? I'm also open for suggestion how we can separate concern better. Sure, as mentioned elsewhere, there should eventually be distinct APIs for each concern. Those APIs do not have to and therefore must not deal for example with persistence-internal IDs or dictate how objects are stored but provide the "concern specific" operations. As that's a more general approach, I really prefer to move this one into draft state and discuss the approach on the dev-ML. — Reply to this email directly, view it on GitHub <#1128 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFRE3SA3HOD77KDTYF7RGHL2U3OUTAVCNFSM6AAAAABYOAQGR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMRZHAYTOOBVGY> . You are receiving this because your review was requested.Message ID: ***@***.***>

flyrain · 2025-03-17T16:50:17Z

As that's a more general approach, I really prefer to move this one into draft state and discuss the approach on the dev-ML.

@snazy, here is the dev mail list: https://lists.apache.org/thread/ghtoydoyqfpd3m5qhscqpbgzqm92cyd9. As @eric-maynard mentioned, it was here for a while.

github-actions · 2025-04-17T02:06:12Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-project-automation bot added this to Basic Kanban Board Mar 6, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 6, 2025

POC: Separate Polaris Entities

e6d553c

flyrain changed the title ~~Separate Polaris Entities POC~~ Separate Polaris Entities Mar 8, 2025

All tests passed

1db510d

flyrain force-pushed the poc-separate-entity branch from abdc52b to 1db510d Compare March 9, 2025 02:40

Yufei Gu added 3 commits March 8, 2025 21:49

Make the EclipseLinkMetastore tests work

f3bc46c

Make the TreeMapMetastoreManager tests work

85cce3a

Reformat

7bc5704

flyrain mentioned this pull request Mar 9, 2025

Policy Store: Add PolicyEntity and PolicyTypes #1133

Merged

Yufei Gu added 9 commits March 9, 2025 00:07

Rename

c0a0b89

Refactor

3a7b209

Refactor PrincipalDao

67e714a

Refactor TaskDao

199b60c

Refactor PrincipalRoleDao

953770d

Use jarkarta Nonnull and Nullable instead of jetbrain ones

d173d1a

Refactor

7451a34

Refactor

25a37f6

Refactor

2438108

flyrain marked this pull request as ready for review March 10, 2025 16:29

flyrain requested review from adutra, ashvina, collado-mike, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, snazy and vvcephei as code owners March 10, 2025 16:29

flyrain requested review from MonkeyCanCode, RussellSpitzer, ebyhr and takidau as code owners March 10, 2025 16:29

flyrain commented Mar 10, 2025

View reviewed changes

eric-maynard reviewed Mar 10, 2025

View reviewed changes

Resolve comments

900dd8e

flyrain commented Mar 11, 2025

View reviewed changes

eric-maynard approved these changes Mar 11, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 11, 2025

snazy requested changes Mar 11, 2025

View reviewed changes

github-project-automation bot moved this from Ready to merge to PRs In Progress in Basic Kanban Board Mar 11, 2025

github-actions bot added the Stale label Apr 17, 2025

github-actions bot closed this Apr 24, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Apr 24, 2025

Separate Polaris Entities #1128

Separate Polaris Entities #1128

Uh oh!

Conversation

flyrain commented Mar 6, 2025

Uh oh!

flyrain Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

singhpk234 Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

sfc-gh-ygu Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

flyrain Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

eric-maynard Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

flyrain Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

flyrain Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

eric-maynard left a comment

Choose a reason for hiding this comment

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain commented Mar 11, 2025

Uh oh!

snazy commented Mar 17, 2025

Uh oh!

eric-maynard commented Mar 17, 2025 via email

Uh oh!

flyrain commented Mar 17, 2025

Uh oh!

github-actions bot commented Apr 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flyrain Mar 11, 2025 •

edited

Loading