roryqi commented on PR #11074:
URL: https://github.com/apache/gravitino/pull/11074#issuecomment-4457898135
> The overall direction here is good — extracting per-entity PO logic into
dedicated classes and using a decorator for hierarchical name translation is a
clean separation. A few observations on the design that might be worth
considering.
>
> **The core tension**
>
> `BasePOStorageOps.getPO/listPOs` currently combines two orthogonal
concerns:
>
> 1. **Name translation** (hierarchical logical ↔ physical) — handled well
by `HierarchicalConventionPOStorageOps`
> 2. **Query routing** (cache-enabled → ID-based; no cache → full-name JOIN)
— currently embedded via `GravitinoEnv.getInstance().cacheEnabled()` in
`BasePOStorageOps`
>
> Mixing these in the base class causes the `Capability` enum + runtime
`UnsupportedOperationException` pattern, the `GravitinoEnv` static dependency
leaking into a storage layer class, and the issue in
`MetadataObjectService.TYPE_TO_STORAGE_OPS_MAP` where raw `*POStorageOps`
instances bypass the naming convention.
>
> **An alternative: expose two explicit SQL methods, route in the service**
>
> ```java
> // BasePOStorageOps: pure SQL delegation, no routing logic
> abstract class BasePOStorageOps<PO, Mapper> {
> // ID-based (cache-enabled path)
> public PO getPOByParentId(Mapper mapper, Long parentId, String name) {
... }
> public List<PO> listPOsByParentId(Mapper mapper, Long parentId) { ... }
>
> // Name-based (full join, no-cache path)
> public PO getPOByFullName(Mapper mapper, NameIdentifier ident) { ... }
> public List<PO> listPOsByNSFullName(Mapper mapper, Namespace ns) { ...
}
> }
> ```
>
> Routing moves back to the service as a single private helper per operation
— not repeated per-method:
>
> ```java
> // SchemaMetaService
> private SchemaPO getSchemaPO(NameIdentifier ident) {
> return SessionUtils.getWithoutCommit(SchemaMetaMapper.class, mapper ->
> cacheEnabled()
> ? ops.getPOByParentId(mapper, getCatalogId(ident),
ident.name())
> : ops.getPOByFullName(mapper, ident));
> }
> ```
>
> Since `ops` is still a `HierarchicalConventionPOStorageOps`, both paths
get name translation automatically. `MetadataObjectService` can store wrapped
instances in its map instead of raw ops, fixing the naming convention bypass.
>
> This eliminates the `Capability` enum, the `GravitinoEnv` dependency in
the base class, and the dispatcher `getPO(Mapper, NameIdentifier)`. Each layer
has one job: name translation in the decorator, SQL in the ops, routing in the
service.
>
> **What stays the same**
>
> `HierarchicalConventionPOStorageOps` as a decorator and the
`*POStorageOps` concrete classes are the right abstraction — they just
shouldn't know about cache state.
I got your point.
I will try solve this issue.
I want to extract a pattern that resolve the duplicated code for cached
operations. Maybe I should extract a helper class for them.
--
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]
