jerryshao commented on PR #11074:
URL: https://github.com/apache/gravitino/pull/11074#issuecomment-4457769220
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.
--
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]
