sungwy commented on code in PR #3681:
URL: https://github.com/apache/polaris/pull/3681#discussion_r2796290376
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java:
##########
@@ -179,12 +237,17 @@ public PolarisResolvedPathWrapper getResolvedPath(
*/
@Override
public PolarisResolvedPathWrapper getPassthroughResolvedPath(Object key) {
- diagnostics.check(
- passthroughPaths.containsKey(key),
- "invalid_key_for_passthrough_resolved_path",
- "key={} passthroughPaths={}",
- key,
- passthroughPaths);
+ if (!passthroughPaths.containsKey(key)) {
+ if (pathLookup.containsKey(key)) {
+ return getResolvedPath(key);
+ }
Review Comment:
I introduced this due to the complexity in introducing `PolarisSecurable` as
a key to the `PolarisResolutionManifest` cache. I'm realizing that there's a
lot of complexity in doing so, because unless all call sites are updated to use
`PolarisSecurable`, we'd expect to have a cache-miss even if we are referring
to the same entity (e.g. `TableIdentifier` vs `PolarisSecurable` wrapper on a
`TableIdentifier`).
I'm thinking on my foot now, and I think keeping `PolarisSecurable` as a
thin wrapper to enforce intent-based authorization input and just leaving it
there, and then keeping the `PolarisResolutionManifest` as-is, and making use
of the existing key types may be a better approach to avoid this hassle of
migrating to the new APIs.
This fallback was here to try to mitigate these cache misses, but I think
that would mean that `passthroughPaths` may yield static and stale result from
the `pathLookup`, which is undesirable.
--
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]
