Conversation
asf-ci-hive
added
tests unstable
tests pending
tests passed
and removed
tests pending
tests unstable
labels
Contributor
Author
|
@ayushtkn if you have some spare time to review, thanks |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
asf-ci-hive
added
tests passed
tests pending
and removed
tests pending
tests passed
labels
ayushtkn
reviewed
Apr 29, 2026
|
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
Member
…e DB to ensure no stale table object is returned;
- improved check; - quiesce console logs due to internal throws in servlet;
… HMSCachingCatalog; - added l1 cache (default 3s / 32 entries) to reduce the latency for repeated access to the same table; - fix license header and addressed review comments;
…cache performance metrics; - remove end point to access cache performance metrics; - enhanced tests to check L1 cache; - simplified MetadataLocator exception handling;
The HMSCachingCatalog serves tables and access decisions out of an in-JVM
cache to avoid HMS round-trips. Caching the catalog this way silently
bypassed Ranger: once a Table object lived in the Caffeine cache, every
subsequent loadTable/dropTable/rename served it without re-consulting the
authorizer, so a user could read or mutate a table they were never granted.
Caching must never widen access. This change makes every table, view and
namespace operation go through an explicit per-request authorization check,
and caches the *decision* (not just the table) so enforcement stays cheap.
Authorization
- New HMSPrivilegeHelper interface: resolves an AccessLevel
(NONE / READ_ONLY / READ_WRITE) for a (db, table, user) or (db, user)
triple, with isAvailable() to report whether an authorizer is wired.
- New RangerPrivilegeHelper implementation calls the Hive authorizer's
showPrivileges API directly (no Thrift hop) and maps Ranger's Hive
access-type names onto AccessLevel:
* read (shared): SELECT, READ
* table/view write: UPDATE, WRITE, ALL (DML / data-plane)
* namespace write: CREATE, ALTER, DROP, ALL (DDL)
ALTER and DROP are DDL and are authorized at the namespace level, not
per-table. Ranger qualifiers (e.g. "SELECT(ACCESS_CONDITIONAL)") are
stripped before matching.
- Fail-closed by default: when no authorizer is configured the helper
returns NONE, so access is denied rather than open. Initialization
failures likewise degrade to NONE. Only when authorization is explicitly
disabled does the helper grant READ_WRITE.
- HMSCachingCatalog enforces READ_ONLY for load/list and READ_WRITE for
drop/rename/register/build on both tables and views, resolving the caller
from UserGroupInformation.getCurrentUser().
Decision caching and invalidation
- Access levels are held in a dedicated Caffeine cache keyed by
TableIdentifier, expiring on the same TTL as the table cache. Namespace
decisions use a synthetic TableIdentifier(namespace, "*") key that cannot
collide with a real table.
- Authorization entries are invalidated together with the object they guard:
table-level on invalidateTable, namespace-level on dropNamespace.
Catalog hardening
- HMSCachingCatalog is now final; its cache callbacks and logger are private.
It is instantiated only by HMSCatalogFactory.
- tableExists uses MetadataLocator (a null location means no table),
avoiding a full load.
Tests
- TestHMSCachingCatalogAuthz drives a StubPrivilegeHelper to assert the
access matrix (grant/deny per level), that decisions are cached, and that
cache invalidation re-checks authorization.
- Surefire runs with reuseForks=false in this module to isolate JVM-static
state (the metastore PMF and Iceberg's CachedClientPool) across classes.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Suppressed comments (5)
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:291
- The namespace path has the same fail-open behavior: an authorizer initialization failure produces an unavailable helper that returns
NONE, but this branch grantsREAD_WRITE. Delegate to the helper so intentionally disabled authorization still uses its pass-through implementation while initialization failures remain denied. Security disposition: VALID under THREAT_MODEL.md §13; direct HMS/REST clients are in scope (§3.3/§4) and authorization scoping is claimed (§5).
if (!privilegeHelper.isAvailable()) {
return AccessLevel.READ_WRITE;
}
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:620
- For a metadata table, invalidating only
db.base.snapshotsleaves the cached origindb.baseat line 630 untouched. The subsequent reload is then overwritten by a metadata-table instance built from that stale origin'sTableOperations, so this mismatch path can return stale metadata again. Resolve metadata identifiers to the base identifier and invalidate the base (which also evicts all derived metadata entries).
invalidateTable(canonicalized);
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:222
- A non-positive L1 size/TTL appears to disable L1, but
Collections.emptyMap()is immutable whileloadTableunconditionally callsl1Cache.put(...)at several paths. With either setting at 0, the first successful load therefore throwsUnsupportedOperationException. Use a writable no-op cache or guard every write when L1 is disabled.
l1Cache = Collections.emptyMap();
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:126
- The documented JMX object name does not match the registration below or
HMSCachingCatalogMXBean: the actual domain/type isorg.apache.iceberg.rest:type=HMSCachingCatalog. Operators following this class documentation will query a nonexistent MBean.
* MBean server under the name {@code org.apache.hive:type=IcebergRESTCatalog,name=<catalogName>}
standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java:609
- When HMS no longer returns this table,
MetadataLocator.getLocationreturnsnull, so this branch refreshes the L1 timestamp and keeps returning the dropped table indefinitely. That directly defeats this PR's stale-object check. Treat a missing metadata location as a mismatch: invalidate and let the underlying load reportNoSuchTableException.
if (location == null) {
LOG.debug("Table {} has no location, returning cached table without location", canonicalized);
onCacheHit(canonicalized);
l1Cache.put(canonicalized, now);
return cachedTable;
- Fail-closed: don't override the privilege helper's NONE with READ_WRITE when !isAvailable(). - Authorize metadata tables (db.tbl.snapshots) against their base table (db.tbl), not a same-named decoy. - loadTable throws NoSuchTableException on a dropped table instead of serving the stale cached instance. - MetadataLocator.getLocation returns null (not throws) for a missing db/catalog, so null uniformly means not-found. - Guard L1 recency-guard writes so they no-op when L1 is disabled (empty map no longer throws). - Log JMX registration failure at error, not warn. - Fix class javadoc to the real MBean ObjectName and note catalog.name() == CATALOG_DEFAULT. - Tests: fail-closed denial, metadata-table authz, L1 disabled, dropped-table reload.
Roll back the AccessLevel-based authorization recently added to HMSCachingCatalog: remove the authz fields, methods, and per-operation guards, delete HMSPrivilegeHelper and RangerPrivilegeHelper, and drop the 3-arg constructor. L1/L2 caching, the JMX MBean, and dropped-table -> NoSuchTableException are unchanged. Per-operation authorization belongs in IcebergAuthorizer, which already does it right for stage-create; extending it to the other operations is deferred to a follow-up PR. Until then, writes and cache-miss reads are authorized by HMS and stage-create by IcebergAuthorizer; only cache-hit reads are unchecked at the catalog level, which the follow-up closes. Tests: replaced TestHMSCachingCatalogAuthz with TestHMSCachingCatalogCache (pure-cache cases, 2-arg constructor).
Make the L1 recency guard access-ordered (LRU) so re-confirming a hot table moves it to the tail and the eldest evicted is the least-recently-used entry, not the least-recently-inserted one. Fix the MetadataLocator.getLocation javadoc, which claimed it returns null for non-metadata tables when it also serves base-table identifiers.
…aLocator Narrows the metadata-location lookup validation to only reject an Iceberg view loaded as a table (throwing NoSuchTableException), matching loadTable semantics, instead of rejecting any non-Iceberg-table object.
LongAdder scales better than AtomicLong under concurrent increments on the cache callback path. The debug log now reads the running total via sum(), guarded by isDebugEnabled() so the write path stays contention-free.
…ingCatalog Extract HMSCatalogFactory.createHiveCatalog so tests build catalogs through the production path, and have the server extension expose newServerCatalog / newCachingCatalog keyed off the metastore's real Thrift URI. Drop the static cacheRef SoftReference, getLatestCache, and the HIVE_IN_TEST hook from HMSCachingCatalog. Rework the caching cache/stats tests to drive their own catalog instance and assert counters via getters and JMX.
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This checks the table location from HMS DB to ensure no stale table object is returned;