Abstract resolution mechanisms from service implementations #514
Conversation
snazy
force-pushed
the
hide-entity-resolution-from-services
branch
7 times, most recently
from
e287e41 to
10d4a70
Compare
snazy
changed the title
snazy
force-pushed
the
hide-entity-resolution-from-services
branch
4 times, most recently
from
ef6390f to
aba25f7
Compare
snazy
changed the title
snazy
requested review from
jbonofre,
ashvina,
RussellSpitzer,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike,
ebyhr and
adutra
as code owners
| throw ex; | ||
| } | ||
| } | ||
| ; |
|
See the doc I attached to #526 . In my mind, much of this code goes away and is replaced by a cache-capable |
Currently all service implementations, both the Polaris admin service and the (Iceberg REST) catalog service need knowledge about the implementation details about _how exactly_ entities and their instances are managed. This is rather a persistence implementation concern, not a service implementation concern. Services should tell, if necessary, tell a properly abstracted builder which entities it will need. Another concern this change tackles is to eventually allow a more efficient data model that is suitable for horizontally scalable databases without having to use pgsql with the implicitly required serializable isolation level. This change abstracts the two related but still different entity resolution (`ResolutionManifest` and `Resolver`) via interfaces and concrete implementation classes, eliminating the hard dependency of services to concrete persistence specific implementations, and also replace the C/Go-style error handling with exceptions. On top, this change moves types to separate packages. This PR is part of a series of changes to abstract implementations and separate those by concern. Follow ups of this change include: * Remove the dependency on "entity cache" from `Resolver` - consumers of `Resolver` unwrap those in all cases * Rather unify `ResolutionManifest` and `Resolver` - those are very tightly coupled things, doing quite some work * Implement proper exception abstraction (the changes in this PR are already better, but not as good as it could be) * Eventually let service implementation work only with their natural keys, and not interact with many types (meta-store-manager-factory, meta-store-manager, meta-store-session, entity-manager, entity-cache).
I don't think that this and the doc really conflict and we agree on the overall effort to encapsulate things better. This one is one "baby step" towards the persistence-model doc shared |
snazy
force-pushed
the
hide-entity-resolution-from-services
branch
from
aba25f7 to
33b6529
Compare
There was a problem hiding this comment.
I like the direction, but I think @dennishuo should take a good look at this.
| ResolutionManifest buildResolved(PolarisEntitySubType subType); | ||
|
|
||
| ResolutionManifest buildResolved(); |
There was a problem hiding this comment.
build or buildManifest
| */ | ||
| package org.apache.polaris.core.persistence.resolution; | ||
|
|
||
| import com.google.errorprone.annotations.CanIgnoreReturnValue; |
There was a problem hiding this comment.
Do we use these annotations anywhere else?
| @@ -1006,6 +1012,20 @@ public Map<String, String> deserializeProperties(PolarisCallContext callCtx, Str | |||
| : new PrincipalSecretsResult(secrets); | |||
| } | |||
|
|
|||
| @Override | |||
| @Nonnull | |||
| public ResolutionManifestBuilder newResolutionManifestBuilder( | |||
There was a problem hiding this comment.
This should be completely separate from the PolarisMetaStoreManager. I think the Resolver process is business logic, not persistence and I think the PolarisMetaStoreManager interface should be focused on persistence only.
| var resolver = primaryResolver.buildResolved(); | ||
|
|
||
| List<PrincipalRoleEntity> activatedPrincipalRoles = | ||
| resolver.getResolvedCallerPrincipalRoles().stream() | ||
| .map(ce -> PrincipalRoleEntity.of(ce.getEntity())) | ||
| .collect(Collectors.toList()); | ||
| this.authenticatedPrincipal.setActivatedPrincipalRoles(activatedPrincipalRoles); |
There was a problem hiding this comment.
Why is this pulled out of the Resolver process? I mean, I don't think it should be in there, but I think it should be part of the construction of the AuthenticatedPolarisPrincipal, not here. I'd scope this change to keep the logic inside the Resolver process for now, then extract the PrincipalRole resolution as part of a new change.
| import org.apache.polaris.core.persistence.cache.EntityCacheByNameKey; | ||
| import org.apache.polaris.core.persistence.cache.EntityCacheEntry; | ||
|
|
||
| final class ResolverImpl implements Resolver { |
There was a problem hiding this comment.
Why not push these fields into the PolarisResolutionManifest? It seems that the ResolverImpl is just a container, but the PolarisResolutionManifestBuilder seems to build a fully resolved manifest. Can we just consolidate these?
Currently all service implementations, both the Polaris admin service and the (Iceberg REST) catalog service need knowledge about the implementation details about how exactly entities and their instances are managed. This is rather a persistence implementation concern, not a service implementation concern. Services should tell, if necessary, tell a properly abstracted builder which entities it will need.
Another concern this change tackles is to eventually allow a more efficient data model that is suitable for horizontally scalable databases without having to use pgsql with the implicitly required serializable isolation level.
This change abstracts the two related but still different entity resolution (
ResolutionManifestandResolver) via interfaces and concrete implementation classes, eliminating the hard dependency of services to concrete persistence specific implementations, and also replace the C/Go-style error handling with exceptions.On top, this change moves types to separate packages.
This PR is part of a series of changes to abstract implementations and separate those by concern. Follow ups of this change include:
Resolver- consumers ofResolverunwrap those in all casesResolutionManifestandResolver- those are very tightly coupled things, doing quite some work