[improvement](hive) Batch Hive metastore partition access - #67186
[improvement](hive) Batch Hive metastore partition access#67186CalvinKirs wants to merge 14 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
changed the title
Issue Number: None
Related PR: None
Problem Summary: Hive tables with very large partition counts could issue one HMS RPC per partition or one unbounded getPartitionsByNames request. This caused excessive serial RPC latency and exposed Thrift message-size, timeout, memory, cancellation, cache-concurrency, and observability gaps. Add one HMS-owned bounded batch loader with adaptive halving for classified Thrift failures, strict result-integrity validation, request deadline and cancellation, physical RPC metrics, and Query Profile events. Reuse it from query, MTMV, statistics, display, and regular write partition-object paths, and coalesce concurrent cache misses per partition with bounded FIFO load slots.
Hive metastore partition-object access is now bounded, cancellable, observable, and shared across query, MTMV, statistics, display, and write paths.
- Test: Unit Test
- HMS batch, cache, and pool tests
- Hive metadata and transaction tests
- MTMV and connector profile tests
- Behavior changed: Yes (large Hive partition-object reads use bounded HMS batches with adaptive fallback and strict integrity checks)
- Does this need documentation: No
00588b2 to
7ab8c53
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes. Three review rounds converged cleanly—all final-round reviewers returned NO_NEW_VALUABLE_FINDINGS—on nine distinct issues: five P1 correctness/compatibility/capacity problems and four P2 lifecycle/observability problems. The review is complete rather than capped.
Critical checkpoint conclusions:
- Goal and proof: the PR establishes the intended common HMS batching, integrity, cancellation, cache-coordination, and telemetry path, and the added unit tests prove many normal/error paths. The nine inline defects prevent the implementation from safely meeting the full contract.
- Scope and clarity: this is a justified but broad 58-file cross-cutting change. The raw-loader/cache/SPI decomposition is generally clear; the remaining defects concentrate at ownership and phase boundaries.
- Concurrency: query/MTMV/display request threads coordinate through per-key in-flight state, FIFO slots, and striped locks while HMS RPCs stay outside locks. Normal owner/waiter cleanup and lock ordering are sound, but refresh can miss a newly registered publisher, cache-disabled traffic bypasses admission, and MTMV local mapping/version capture is not atomic.
- Lifecycle/static state: task-owned
StatementContextcleanup and successful connector replacement are sound. Temporary validation contexts and superseded failed-init contexts leak shared metrics references. No C++ static-initialization concern applies. - Configuration: the new HMS batch/fallback properties are validated and wired consistently through Hive and Hudi; they are catalog-scoped rather than dynamic process variables. Disabling partition retention incorrectly disables the pool-derived admission bound.
- Compatibility: the public connector SPI surface changes without the repository-mandated 7.0 major bump, so the major-only plugin gate permits incompatible 6.x linkage. No storage-format or FE/BE protocol change is present.
- Parallel paths: all eight production partition-object callers and Hive/Hudi construction were traced. Query/write/statistics sources and sessions are propagated; SHOW/display purpose is not, and the analogous task/display MTMV phase split needs the same ownership fix.
- Conditions and error handling: strict identity/order validation, typed integrity failures, statement deadlines, and cancellation/pool-taint boundaries are otherwise sound. The generic
TTransportExceptioncondition is too broad, and the MTMV outside-lock predicate is too broad. - Tests and results: changed tests cover batching, fallback state, result integrity, owner/waiter cleanup, cancellation, metrics/profile aggregation, and MTMV bulk caching. Missing decisive coverage is called out inline for refresh registration, disabled-cache zero-pool concurrency, mixed/cloud mapping races, context ownership, DISPLAY, outer logical-event aggregation, and process wait metrics. Per the review prompt, I ran no build or tests. Current CI has green style/security/coverage contexts; FE UT, compile, performance, and automated review are still pending.
- Observability: event schema/cardinality and profile serialization are generally sound, but context leaks, DISPLAY mislabelling, caller-event misaggregation, and missing process wait metrics make the advertised telemetry inaccurate.
- Transactions, persistence, writes, and FE/BE variables: no transaction protocol, persisted schema, EditLog payload, storage format, or FE-to-BE variable was added. Replay initialization and Hive write/transaction callers were audited; their accepted issue is the shared context lifecycle leak, not transaction atomicity.
- Performance: bounded physical batches and single-flight improve the intended large-partition path, but ordinary outages can be amplified through the halving ladder and cache-disabled zero-pool traffic can open unbounded concurrent clients.
- Other applicable checks: this PR is FE Java only, so BE memory tracking, nullable-column handling, delete bitmaps, and C++ static/TU rules are not applicable. No additional issue survived the final changed-file and missed-path sweep.
No additional user-provided review focus was supplied.
| this.rawStoragePropsSupplier = | ||
| Objects.requireNonNull(rawStoragePropsSupplier, "rawStoragePropsSupplier"); | ||
| this.environment = buildEnvironment(); | ||
| this.metadataAccessMetrics = new ConnectorMetadataAccessMetrics(catalogName); |
There was a problem hiding this comment.
[P2] Give the validation metrics reference an owner. This constructor now acquires a shared per-catalog metrics reference even for the temporary context created by forCatalogCreationValidation(). CatalogFactory passes that context inline and retains only the connector, while PluginDrivenExternalCatalog explicitly leaves connectorContext null for this validation connector, so neither initialization nor catalog teardown can call DefaultConnectorContext.close() on it. Each create/replay attempt therefore leaves an entry in SHARED_METRICS; after the live catalog records metrics, DROP also cannot unregister those catalog-labelled series because the leaked reference keeps the count nonzero. Please make validation use a non-acquiring metrics sink or give the temporary context an explicit owner that closes it on every success/failure/fallback path. The same ownership rule is also needed for live initialization: construct into a local context, publish it only after connector creation succeeds, and close it on null/throw so repeated retries cannot overwrite and leak failed contexts.
| } | ||
| // Write binding gained execution-capability methods in this surface revision. A plugin built against | ||
| // major 5 must be refused rather than run against a contract it did not compile against. | ||
| // Write binding gained execution-capability methods, while metadata access gained operation control, |
There was a problem hiding this comment.
[P1] Bump the connector SPI major for this surface change. This PR adds methods and types to the public connector SPI, but the API is still stamped as 6.0. The policy beside connector.plugin.api.version requires a same-commit major bump for any SPI surface addition, and ApiVersionGate checks only major equality. As written, a plugin compiled against these new APIs is labelled 6.0 and can be accepted by an older 6.0 FE, then fail at first use with NoSuchMethodError/NoClassDefFoundError. Please bump the connector API major (and this assertion) to 7.0 in this commit.
There was a problem hiding this comment.
Thanks for calling this out. We intentionally will not bump the connector SPI to 7.0 in this PR. The compatibility boundary should be a published connector API version, not every PR that evolves an API which is still unreleased on master. Connector SPI 6.0 was introduced on master on Aug 17, 2026, and no release tag contains that commit, so 6.0 is still the next unpublished surface; this PR updates that same pre-release surface and its frozen baseline before publication. Once 6.0 is released, a subsequent incompatible surface change must bump the major. Bumping the major once per pre-release PR would consume versions without creating a real artifact compatibility boundary. The existing major gate still correctly separates published/older major 5 plugins from the upcoming major 6 API.
| try { | ||
| invalidateInFlightPartitionLoads(key -> key.matches(dbName, tableName), true); | ||
| } finally { | ||
| stateLock.unlock(); |
There was a problem hiding this comment.
[P1] Keep registration fenced through the cache clear. The state lock is released before partitionsCache.invalidateIf() bumps the generation. A cold request can therefore register after the in-flight scan, start its HMS RPC, then let this refresh clear the cache and return; because that new batch was never marked invalid and publishOwnedPartitions() uses a direct put, its pre-clear load is cached afterward for the full TTL. The same gap exists in partition/DB/catalog invalidation. Please perform the matching cache invalidation under the same stripe(s), or capture/check a refresh epoch at owner publication, and add the mark/register/clear/publish interleaving to the concurrency tests.
| } | ||
| for (Throwable current = failure.getCause(); current != null; current = current.getCause()) { | ||
| String className = current.getClass().getName(); | ||
| if (className.endsWith(".TTransportException")) { |
There was a problem hiding this comment.
[P1] Do not halve batches for every transport outage. This class-name check makes a closed/refused/reset/EOF/timeout TTransportException degradable even though reducing the payload cannot repair the connection. With the defaults, one 5,000-name offset can be replayed 13 times down to size 1 within the 30-second budget, and each logical call sits above Hive's own retry/reconnect proxy and may create/taint another client. That amplifies an HMS outage precisely while it is unhealthy. Please restrict fallback to explicit frame/message/request/partition-limit signals (or a proven oversize transport code), and make ordinary transport failures terminate after the original logical attempt.
| int start = 0; | ||
| private void loadMissingPartitions(HmsPartitionRequest request, List<String> initialMissNames, | ||
| Map<List<String>, HmsPartitionInfo> resultByIdentity) { | ||
| if (!partitionsCache.isEffectiveEnabled()) { |
There was a problem hiding this comment.
[P1] Preserve load admission when partition caching is disabled. This early return skips both single-flight retention and the new window/slot limiter. In the supported hive.metastore.client.pool.size=0 configuration, the constructor deliberately converts zero to one cold-load slot, but every disabled-cache request now bypasses that slot and ThriftHmsClient creates a fresh client per call; N concurrent scans/freshness probes can therefore open N HMS connections. Please keep windowing and slot admission on this path while skipping only cache coordination/publication, and cover zero-pool plus disabled cache concurrently.
| MTMVPartitionUtil.addPartition(mtmv, partitionKeyDesc); | ||
| } | ||
| } | ||
| boolean buildContextUnderLock = Config.isNotCloudMode() |
There was a problem hiding this comment.
[P1] Keep local PCT mappings atomic with their versions in mixed MVs. This condition moves the entire context build outside the sorted table locks whenever any base table is MVCC. If the actual PCT table is a local OlapTable, its mapping is copied here at T1, external preload can then block, and the later locked refreshLocalBaseVersions() refreshes only versions—not partitionMappings. A local partition dropped in that window remains in the mapping and makes the locked version lookup fail; an added partition is omitted from comparison/refresh. The base code built both together under the locks. Please split the capture so external pins/I/O stay outside, while local PCT mappings and versions are rebuilt together under the sorted FE locks. Cloud local-only plans also always take this branch and the refresh helper is a no-op there, so preserve an atomic cloud capture as well. Apply the same fix to the analogous PartitionsProcDir branch and add mixed local-PCT/external-MVCC plus cloud local-only race tests.
| } | ||
| HiveTableHandle hiveHandle = (HiveTableHandle) handle; | ||
| List<HmsPartitionInfo> partitions = hmsClient.getPartitions( | ||
| session, HmsPartitionAccessSource.MTMV, |
There was a problem hiding this comment.
[P2] Preserve the display source in freshness telemetry. SHOW PARTITIONS now builds and preloads MTMVRefreshContext, reaches these freshness methods, and is always emitted as MTMV here; the sibling whole-table freshness call is hard-coded the same way. There is no production use of the newly added HmsPartitionAccessSource.DISPLAY, so display traffic is indistinguishable from refresh/rewrite work in both process metrics and Query Profile despite the per-source observability contract. Please thread the logical access purpose into this freshness request and emit DISPLAY for the proc/display path, with a production-chain test.
| request, initialMissNames, partitionsCache.invalidationGeneration(), resultByIdentity); | ||
| return; | ||
| } | ||
| for (int offset = 0; offset < initialMissNames.size(); offset += partitionLoadWindowSize) { |
There was a problem hiding this comment.
[P2] Emit one logical event for the caller's request. The cache splits one business request into partitionLoadWindowSize windows and each copied request invokes the raw loader, whose finally records a completed logical event. A cold 12,000-name call therefore increments LogicalRequests three times; if the third window fails, the single caller-visible request is reported as two successes plus one failure. The cache-disabled path reports the same call once, so success rates and requested-item metrics depend on cache configuration rather than business semantics. All-hit and pure-waiter calls emit no completed business event, and mixed hit/miss calls report only owner misses. Please move event ownership to the outer cache call, aggregate the shared business-request state, and emit exactly once while retaining physical-attempt counters; test all-hit, mixed, pure-waiter, and multi-window later-failure cases.
| .success(success) | ||
| .build(); | ||
| try { | ||
| request.getMetadataAccessObserver().record(event); |
There was a problem hiding this comment.
[P2] Publish coordination waits to process metrics too. Both in-flight and cold-load-slot wait events are recorded only through the request observer, which is a Query Profile sink and becomes NOOP when profiling is disabled. The catalog observer used by the raw loader never reaches this cache layer, so FE metrics permanently omit both newly advertised wait operations even while reporting their downstream HMS call. Please give the cache the catalog observer and safely publish these events to both sinks, with process-metric coverage when query profiling is disabled.
TPC-H: Total hot run time: 16850 ms |
TPC-DS: Total hot run time: 81114 ms |
ClickBench: Total hot run time: 14.48 s |
FE UT Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Request changes. Three review rounds converged with four additional issues: one P1 cancellation/lifecycle gap and three P2 compatibility/performance gaps. I did not duplicate the eight existing issue threads; their unresolved blockers remain part of this verdict. The live PR still matches the authoritative bundle at head 7ab8c5329b97a6ac3e9361e593383e5ac0bf6bd8. Current CI has compile, FE UT, CheckStyle, P0, non-concurrent, vault, and performance checks passing, while External Regression and cloud_p0 are failing.
Critical checkpoint conclusions:
- Goal and proof: The PR implements batched/cached HMS partition freshness, cooperative cancellation, telemetry, and MTMV bulk snapshot loading, and its unit tests prove many component paths. It does not fully achieve the stated bounded/large-scale behavior because client construction remains outside cancellation/deadline control, the cache-backed scale path repeats canonicalization, and MTMV can eagerly load a huge union before a locally decisive stale gate.
- Scope and focus: The 58-file connector/HMS/MTMV change is internally related but not yet safely mergeable. The user focus file contained no additional focus request; the full PR was reviewed.
- Concurrency and thread safety: Enabled-cache owner/waiter futures, permits, publication, retry cleanup, and lock ordering otherwise balance. Existing threads already cover the cache-invalidation fence and disabled-cache admission bypass; the new P1 below covers synchronous client creation before cancellation can act. Heavy external work is generally moved outside FE locks, subject to the existing mixed local/cloud atomicity thread.
- Error handling: Strict result-integrity failures and cancellation propagation are fail-loud and preserve causes in the inspected paths. The existing broad transport-fallback thread and the new eager-preload ordering can still amplify or surface avoidable HMS failures.
- Lifecycle: Watchdog ThreadLocal cleanup, interrupt ownership, pooled-client taint/return, statement pins/scopes, and normal connector-context close were traced. Existing metrics-reference ownership remains a live thread; any fix for client creation must destroy a late result after cancellation, deadline, or concurrent close.
- Configuration and dynamic behavior: Hive and Hudi bind the same positive batch/timeout properties and defaults through catalog construction/replay. No additional dynamic-update divergence survived review.
- Compatibility and rolling upgrade: Default SPI methods preserve old implementation linkage, and the existing API-major thread includes the unreleased-6.0 context. Separately, the frozen-surface test omits the new reachable session/control/observer/event/abort contracts and metadata return types, so future incompatible drift can evade the gate.
- Parallel paths: Query, statistics, MTMV, and write callers plus Hive/Hudi construction were checked. Rewrite, task, metadata/global sync, and proc/display MTMV paths were all traced. The existing DISPLAY-source thread remains the only distinct source-label issue.
- Special conditionals: Excluded-table and PCT-first comparison semantics are intentional. Existing review context covers transport degradability and cache-disabled branching; the new MTMV finding covers preload ordering before the name-set condition.
- Test coverage: Added tests cover batching, strict ordering, cache coordination, pool wait cancellation, metrics/profile aggregation, context capture, and 160k aggregation. Missing cases are identified inline: blocking client creation, frozen reachable SPI contracts, cache-backed parse counts, and large name-set mismatch with zero freshness calls.
- Test results: This review-only environment expressly prohibited builds/tests, so none were run here. No regression
.outfiles changed. Live FE UT/compile/style checks pass, butExternal Regressionandcloud_p0currently fail. - Observability: Process metrics and Query Profile coverage were inspected. Existing threads cover metric reference ownership, fragmented logical events, missing process wait metrics, and DISPLAY attribution; no additional observability issue survived.
- Transaction and persistence: MTMV refresh snapshot generation, manual/COMPLETE refresh, current-relation resolution, and per-partition persistence inputs were traced. No new EditLog schema is introduced; the existing MTMV mapping/version atomicity thread remains applicable.
- Data writes and crash behavior: No new BE/storage data-write path is introduced. MTMV refresh scheduling and snapshot capture were checked; no distinct crash leak or partial-write issue survived beyond the live atomicity/lifecycle threads.
- FE/BE variables: No new FE-to-BE variable or protocol field is introduced.
- Memory safety and nullable handling: The change is Java/FE-only; BE allocator, C++ lifetime, and nullable-column checkpoints are not applicable. Java ownership and large temporary allocations were reviewed, with the repeated identity allocation issue called out inline.
- Data correctness: Strict partition identity, duplicate, missing, unexpected, and ordering checks are coherent. Existing threads cover cache freshness fencing and MTMV atomicity; the dismissed display snapshot split predates this PR.
- Performance: Batching removes per-partition RPCs, but the cache-backed request performs
3Nparses on all hits and6N+Cwhen fully cold, and MTMV may issue a 160k-name freshness load before a set mismatch already proves staleness. - Other issues and completion: All candidates are accepted, deduplicated, or dismissed with code evidence. Round 3 ended with
NO_NEW_VALUABLE_FINDINGSfrom both normal full reviews and the independent risk review, so this review is complete.
| waitMillis = Math.min(waitMillis, operationRemainingMillis); | ||
| } | ||
| try { | ||
| return clientPool.borrowObject(waitMillis); |
There was a problem hiding this comment.
[P1] Bound HMS client creation with the operation control. On an empty pool, Commons Pool 2.2 runs HmsClientFactory.create() synchronously inside borrowObject(waitMillis) before the timed idle-object wait, so waitMillis does not bound createFreshClient(); the pool-disabled branch calls it directly as well. Kerberos login, DNS, or socket construction can therefore remain stuck after KILL/deadline, before HmsRemoteCallTracking installs its watchdog and before the next checkActive(). Please make creation cancellable/deadline-aware (and destroy any client that completes late) for both branches, with blocking-provider KILL/deadline tests.
| return ConnectorStatementScope.NONE; | ||
| } | ||
|
|
||
| /** Returns cooperative cancellation and deadline control for connector metadata operations. */ |
There was a problem hiding this comment.
[P2] Freeze the new session/control API in the plugin surface. ConnectorPluginSurfaceTest.FROZEN_TYPES does not include ConnectorSession or the new control/observer/event/abort types, so the regenerated baseline records ConnectorContext#getMetadataAccessObserver() but not these two session methods or the callable contracts they expose. The separate metadata baseline also omits return types. That leaves later removal/re-signing of this new 6.0 surface invisible to the stated compatibility speed bump. This is independent of whether 6.0 is still unpublished: please freeze these reachable SPI types (or recursively freeze reachable SPI contracts), regenerate the baseline, and assert the new methods are present.
| operationControl.checkActive(); | ||
| } | ||
| String partitionName = partitionNames.get(i); | ||
| HmsPartitionIdentity.ParsedPartitionName parsed = HmsPartitionIdentity.parse(partitionName); |
There was a problem hiding this comment.
[P2] Retain parsed identities across the cache-backed request. This builder validates every partition name with HmsPartitionIdentity.parse() and then discards the result. The normal cold-cache path reparses all names during cache lookup, registration, copied-window construction, raw validation, and final reconstruction—6N+C parse/unescape passes for a fully cold sole owner (roughly 720k canonicalizations for 120k names); even all hits take 3N. The added 120k test covers only the raw loader, so it misses this decorator cost. Please carry immutable parsed keys/values on the request and share/slice them through windows and validation, with a cache-backed large-request parse-count/performance test.
| throws AnalysisException { | ||
| MTMV mtmv = context.getMtmv(); | ||
| Set<String> partitionNames = mtmv.getPartitionNames(); | ||
| context.preloadSnapshots(partitionNames, tables, excludeTables); |
There was a problem hiding this comment.
[P2] Check the persisted partition set before eager freshness loading. This preload fetches partition-detail freshness for the entire mapped PCT union before isSyncWithPartitions() compares the current names with the persisted getPctSnapshots() set. With a coarse mapping over 160k Hive partitions, adding or dropping one partition already proves the MTMV stale locally, but this order now issues the full HMS freshness request first (and can fail before returning that deterministic stale result); the pre-PR path performed the set gate before fetching snapshots. Please make the comparison two-phase—reject name-set mismatches first, then preload only mappings that still need version checks—and add large mismatch coverage asserting no partition-freshness call.
|
/review |
There was a problem hiding this comment.
Reviewed the authoritative 58-file bundle and all live/raw review comments through three convergence rounds. Two additional non-duplicate issues remain, alongside the unresolved live blockers, so I am requesting changes.
Key checkpoints: the cross-layer scope matches the stated bounded/cancellable HMS-load and MTMV-freshness goal, and the new request, loader, control, telemetry, and refresh-context helpers generally keep responsibilities clear. Strict response validation/order, typed cancellation, cache/pool cleanup, connector construction, statement-scope closure, and the main query/statistics/scan/write/refresh/rewrite/display paths were traced. The remaining new gaps are (1) rewrite eagerly loading locally rejectable candidates while planner locks are held and (2) equivalent waiters serially replaying a shared integrity failure. Existing threads already fence the other identified concurrency, invalidation, compatibility, metrics, and eager-loading concerns. Hive/Hudi configuration validation and SPI forwarding were checked; this patch changes no storage/EditLog format, transaction protocol, FE/BE wire value, or BE memory/nullability path.
No local build or tests were run because the review bundle forbids them. Current checks show FE UT, compile, P0, nonconcurrent, and vault passing; External Regression and cloud_p0 are failing, with no public failure detail available from the linked TeamCity endpoints. The changed unit tests cover most raw/cache/control and MTMV batch paths, but not the two concurrent/production-chain cases called out inline.
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: The shared Hive metastore partition batch path still had lifecycle, concurrency, compatibility, and MTMV freshness edge cases found during review. Blocking HMS client construction could outlive cancellation, parsed identities were repeatedly rebuilt in cache windows, equivalent waiters could replay terminal integrity failures, and rewrite or refresh paths could preload metadata before deterministic stale gates. This change makes client creation operation-controlled, reuses immutable parsed identities, scopes waiter retries, applies persisted-set gates before freshness loading, and completes SPI and metrics lifecycle coverage. ### Release note Hive metastore partition batching now has stronger cancellation, concurrent failure sharing, MTMV freshness gating, and connector SPI compatibility checks. ### Check List (For Author) - Test: Unit Test - Targeted FE unit tests for HMS cache concurrency, pool control, batch loading, MTMV rewrite and refresh, metrics, and Connector SPI surface - Behavior changed: Yes. Equivalent cache waiters share terminal integrity failures, narrower waiters retry independently, and MTMV rewrite skips freshness loads for locally rejected candidates. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: The new connector metadata access source import was placed before lexicographically earlier HMS and connector SPI imports in three Hive connector classes, causing the full FE CheckStyle workflow to fail. Reorder those imports according to the Doris CustomImportOrder rule. ### Release note None ### Check List (For Author) - Test: Unit Test - cd fe && mvn clean checkstyle:check - Behavior changed: No - Does this need documentation: No
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for four distinct current-head issues: unbounded outstanding HMS client creators, MTMV persistence losing union batching on first/incomplete or mapping-changed baselines, cloud version RPCs under FE table locks, and nested wait spans inflating the legacy metadata-time total.
Checkpoint conclusions:
- Goal, focus, and scope: the common bounded HMS path is broadly wired across query, statistics, MTMV, display, transaction, and write callers; no extra user focus was supplied. The first-refresh persistence case still misses the central batching goal.
- Correctness, conditions, and parallel paths: strict identity/order validation and migrated Hive/Hudi caller paths were checked, including cache-enabled/disabled, pooled/unpooled, first/incomplete/changed MTMV snapshots, local/external/cloud, and success/failure/cancellation branches.
- Concurrency and lifecycle: cache owner/waiter, invalidation, slot, retry, and publication paths were reviewed. The asynchronous creation fix has no bound on interrupt-ignoring creator tasks.
- Configuration and compatibility: new property defaults/validation, source/control defaults, connector SPI freezes, and the unreleased-6.0 compatibility context were checked. No new FE-BE wire dependency was introduced.
- Observability and performance: detailed operation counters remain useful, but the legacy query total double-counts nested waits. The first/incomplete MTMV path can turn a 160k disjoint mapping into roughly 160k logical one-name freshness requests.
- Transactions, persistence, writes, and atomicity: transaction/write callers use the common API; snapshot persistence has the separate preload-mode bug below, and cloud recapture performs remote work inside metadata locks. Existing live atomicity threads were treated as duplicate fences.
- Tests/results: reviewed the changed unit tests and the PR's reported 250-partition manual profile. No build or test command was run in this review, as required by the review task; the PR also states the 120k end-to-end case was not rerun. Missing focused coverage is called out inline.
A complete 63-file final sweep and a second full convergence round found no additional distinct issues beyond these four and existing review threads.
TPC-H: Total hot run time: 17284 ms |
TPC-DS: Total hot run time: 83713 ms |
ClickBench: Total hot run time: 14.96 s |
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Review found four remaining correctness and resource-control gaps in the shared HMS partition batch work. Nested coordination spans double-counted legacy profile time, non-cooperative HMS client creation could grow unbounded daemon threads, MTMV snapshot persistence skipped union preloading for first or changed baselines, and cloud MTMV version refreshes could issue remote calls while FE table locks were held. Count only top-level HMS access in legacy totals, retain bounded client-creation admission until real creator exit, split persistence preloading from comparison gating, and preload cloud MTMV versions before locks while revalidating locked structure from raw local caches. ### Release note Improve HMS partition batch profiling, cancellation resource bounds, MTMV snapshot batching, and cloud lock behavior. ### Check List (For Author) - Test: Unit Test - SummaryProfileTest, ThriftHmsClientPoolControlTest, MTMVRefreshContextBatchTest, MTMVPartitionUtilTest, MTMVRewriteUtilTest, PreloadExternalMetadataTest (46 tests) - MTMVTaskTest and PartitionsProcDirTest (18 tests) - cd fe && mvn clean checkstyle:check - Behavior changed: Yes. Legacy profile totals no longer double-count nested waits, HMS creators are bounded, persistence uses ungated union preloading, and cloud version RPCs run before FE table locks. - Does this need documentation: No
There was a problem hiding this comment.
Requesting changes for nine distinct current-head issues. Review convergence is capped/incomplete: the maximum third round found the outer Kerberos authentication gap (the ninth inline comment), so the review protocol does not allow a fourth round to prove clean convergence. All nine findings were independently validated and deduplicated against bundled and live review threads.
Critical checkpoint conclusions:
- Goal and proof: the PR broadly implements shared bounded HMS partition loading, cancellation/deadlines, strict integrity, telemetry, and MTMV freshness preloading. The added unit tests prove many component paths, but the nine gaps below prevent the end-to-end contract from being complete.
- Scope and focus: the 66-file FE connector/core scope is broad but related and reasonably decomposed. No additional user focus was supplied; the full PR was reviewed.
- Concurrency and locks: cache owner/waiter publication, FIFO admission, invalidation stripes, permits, and MTMV table-lock order were traced; no additional deadlock or stale-publication issue survived current-head fixes. Remote MTMV work is generally outside FE locks. Three separate pre-wire/authentication phases still cannot abort promptly.
- Lifecycle and memory ownership: creator slots, late-client destruction, pooled-client return, connector contexts/observers, statement scopes, and MTMV task cleanup were checked. Prepared statements retain the new refresh context across execution resets, and background parsed executors are detached from connector cancellation. This is FE Java only; BE allocator, C++ static-initialization, and nullable-column checks do not apply.
- Configuration and dynamic behavior: Hive/Hudi batch, fallback, timeout, pool, and cache properties are consistently validated and catalog-scoped; no separate dynamic-update defect survived.
- Compatibility: SPI defaults, forwarding, and frozen-surface tests were checked. The existing unreleased-6.0 SPI-major discussion is a hard duplicate fence. No storage format, EditLog schema, or FE-BE wire protocol is changed.
- Parallel paths: scan, statistics, freshness, write, transaction, task, display, rewrite, scheduled insert, streaming insert, pooled/unpooled, Hive/Hudi, cloud/non-cloud, and prepared execution paths were traced. The sessionless name-list path and parsed background tasks miss the new control propagation.
- Conditions, error handling, and data correctness: strict partition identity/order/result checks and oversize-only fallback are coherent. The remaining defects are wrong exception boundaries, cancellation swallowing, and MTMV mapping/freshness state from different pins or executions.
- Test coverage: changed FE unit tests cover batching, fallback, integrity, cache coordination, creation/pool waits, metrics/profile, and successful MTMV preload. Missing negative coverage maps directly to the inline comments: name-list control, optional-MV failure, planner abort propagation, pin interleaving, parsed background cancellation, reconnect/relogin/outer-auth blocking, and prepared re-execution.
- Test results: no build or test command was run because the review prompt expressly prohibits it. No regression result file changed; the PR also states the 120k end-to-end case was not rerun.
- Observability: logical/physical/wait event aggregation, shared catalog metrics, source labels, profile serialization, and cleanup were checked; existing telemetry threads remain duplicate fences and no new observability-only issue survived.
- Transactions and persistence: shared connector transaction binding and MTMV persistence-union preloading were traced. Existing persistence/atomicity threads fence prior issues; no new EditLog or failover defect was found.
- Data writes and crash behavior: connector rewrite/write transaction flow is otherwise coherent, but detached cancellation can let a cancelled background rewrite/insert continue before coordinator creation. No BE crash or storage atomicity path is added.
- FE-BE variables: no new transmitted variable or scattered thrift send path is introduced.
- Performance: central batching/cache reuse materially improves the large-partition path. The full sessionless name list and duplicate/unpinned MTMV materialization remain problematic as described inline; no other distinct CPU, memory, or RPC amplification issue survived.
- Other/completion: all ledger candidates are accepted, explicitly duplicated, or dismissed with code evidence; all 66 changed paths match the authoritative diff. Because Round 3 produced a new accepted issue, this review is truthfully capped/incomplete rather than claiming full convergence.
| } | ||
| List<String> partNames = hmsClient.listPartitionNames( | ||
| handle.getDbName(), handle.getTableName(), MAX_PARTITIONS_FOR_STATS); | ||
| handle.getDbName(), handle.getTableName(), ALL_PARTITIONS); |
There was a problem hiding this comment.
[P1] Keep the full name listing under the new operation control. This path first calls the sessionless listPartitionNames(..., -1), whose Thrift implementation uses ConnectorOperationControl.NONE; the session is only applied to the later sampled getPartitions call. On a cold very-large table, KILL or the statement deadline therefore cannot stop the largest HMS RPC in this statistics path (and an already-cancelled cached request still copies/shuffles the full list). Please add a session-aware name-list path and use it here and in the parallel scan/write/freshness callers, with blocking cold-list cancellation/deadline coverage.
There was a problem hiding this comment.
Resolved by scope reduction in 9487b21. Query-level cancellation/deadline propagation has been removed from this PR, including ConnectorOperationControl and all session/control wiring. The PR description now explicitly lists name listing and the complete HMS cancellation lifecycle as out of scope.
| } | ||
| try { | ||
| statementContext.putPreloadedMtmvRefreshContext(mtmv, MTMVRefreshContext.buildContext(mtmv)); | ||
| } catch (AnalysisException e) { |
There was a problem hiding this comment.
[P1] Preserve the optional-MV failure boundary here. buildContext can materialize an external PCT snapshot and throw an unchecked DorisConnectorException, but this new unconditional cloud stage catches only AnalysisException. A query over a healthy base table can now fail because an otherwise optional candidate MTMV references an unavailable connector; the later rewrite hook historically degrades such a candidate to no rewrite. Please rethrow ConnectorOperationAbortedException, but isolate/log ordinary connector failures per candidate, with a cloud collect-stage failure test.
There was a problem hiding this comment.
Fixed in 9487b21. The optional cloud MTMV candidate boundary now catches AnalysisException and DorisConnectorException per candidate, logs the failure, and continues base-table analysis. PreloadExternalMetadataTest covers this connector-failure path.
| } | ||
| Set<TableNameInfo> excludeTables = forceConsistent | ||
| ? ImmutableSet.of() : mtmv.getQueryRewriteConsistencyRelaxedTables(); | ||
| refreshContext.preloadSnapshots(mtmvNeedComparePartitions, |
There was a problem hiding this comment.
[P1] Do not let the async-MV hook swallow operation aborts from this preload. The table boundary deliberately rethrows ConnectorOperationAbortedException, but it passes this AnalysisException-only catch and is then caught by createAsyncMaterializationContext's catch (Exception), which returns an empty MV list. If KILL/deadline fires here before a coordinator exists, planning can continue into a local base plan despite the executor's cancel flag. Please explicitly rethrow operation aborts at the hook boundary and add CANCELLED and DEADLINE_EXCEEDED planner-hook tests.
There was a problem hiding this comment.
Resolved by scope reduction in 9487b21. ConnectorOperationAbortedException and query-level cancellation/deadline propagation were removed from this PR, so there is no operation-abort exception for this hook to swallow.
| */ | ||
| public ExternalMetadataPreloadResult executePreload(StatementContext statementContext) { | ||
| long preloadStartTime = TimeUtils.getStartTimeMs(); | ||
| preloadCloudMtmvRefreshContexts(statementContext); |
There was a problem hiding this comment.
[P1] Build this cloud context from the statement's exact external snapshot. This call runs before normal snapshot loading, so getAndCopyPartitionItems(empty) materializes an unrecorded latest pin; later freshness uses the statement pin (or materializes latest again), while locked revalidation keeps the old external mapping. A partition added between those reads can be absent from the mapping yet compared against a newer generation, allowing an incomplete MTMV rewrite. Please install/reuse the candidate table pin before building the context and add an interleaving test proving mapping, freshness, and scan share it.
There was a problem hiding this comment.
Fixed in 9487b21. Before building each cloud MTMV refresh context, the preload stage now installs/reuses the PCT table snapshot through StatementContext.loadSnapshots. The added test verifies snapshot loading precedes MTMVRefreshContext.buildContext, keeping mapping/freshness/scan on the statement pin.
| long startMillis = ctx.getStartTime() > 0 ? ctx.getStartTime() : System.currentTimeMillis(); | ||
| long deadlineMillis = startMillis + ctx.getExecTimeoutS() * 1000L; | ||
| // The connection may execute another statement later; bind cancellation to this session's statement. | ||
| StmtExecutor originatingExecutor = ctx.getExecutor(); |
There was a problem hiding this comment.
[P1] Do not capture null cancellation state for parsed-statement background tasks. ConnectorRewriteGroupTask, InsertTask, and StreamingInsertTask construct StmtExecutor(ctx, StatementBase), whose constructor never registers itself on ctx; their cancel paths mark only that executor. A connector session built during source/sink planning therefore captures null here and its cache/pool/retry waits continue until the unrelated deadline. Register every parsed executor before connector planning (centrally or at each task) and cover cancellation while background rewrite/insert metadata is blocked.
There was a problem hiding this comment.
Resolved by scope reduction in 9487b21. Connector session cancellation capture and the StmtExecutor cancellation state added by this PR were removed. Background-task cancellation propagation is no longer claimed by this PR and is explicitly out of scope.
|
|
||
| @Override | ||
| public void reconnect() throws MetaException { | ||
| HmsRemoteCallTracking.checkReconnectActive(); |
There was a problem hiding this comment.
[P1] Track the pre-wire reconnect as an interruptible unsafe phase. Between these two checks, super.reconnect may close the old transport and block in URI resolution/Kerberos/transport open, but neither retryingAfterWireFailure nor clientUnsafeAfterReconnectFailure is set. The watchdog therefore records only a generic abort and cannot return KILL/deadline until setup finishes. Mark reconnect active before entering super; if cancellation interrupts it mid-transition, taint that client. Add a blocking socket-lifetime pre-first-wire reconnect test.
There was a problem hiding this comment.
Resolved by scope reduction in 9487b21. The reconnect cancellation/watchdog lifecycle was removed together with query-level HMS cancellation support. TrackingHiveMetaStoreClient now tracks only physical getPartitionsByNames attempts for observability.
| if (finished || abort != null) { | ||
| return; | ||
| } | ||
| if ((retryingAfterWireFailure || clientUnsafeAfterReconnectFailure) && !wireCallActive) { |
There was a problem hiding this comment.
[P1] Treat initial proxy authentication as a pre-wire setup phase. Hive 3.1.3 calls reloginExpiringKeytabUser() before reconnect and before method.invoke reaches the tracked raw client. While a due keytab renewal blocks on UGI/KDC, all three phase flags are false, so this branch only records a generic abort and KILL/deadline cannot return until login finishes. Model that phase explicitly (or use bounded asynchronous abandonment while retaining the borrowed client until exit) and add blocking first-attempt keytab KILL/deadline tests.
There was a problem hiding this comment.
Resolved by scope reduction in 9487b21. The query cancellation watchdog and its phase model were removed. Authentication cancellation is no longer claimed and is explicitly outside this PR scope.
| // Record mtmv and valid partitions map because this is time-consuming behavior | ||
| private final Map<BaseTableInfo, Collection<Partition>> mvCanRewritePartitionsMap = new HashMap<>(); | ||
| // Cloud MTMV versions are loaded before planner table locks and revalidated from their local caches later. | ||
| private final Map<BaseTableInfo, MTMVRefreshContext> preloadedMtmvRefreshContexts = new HashMap<>(); |
There was a problem hiding this comment.
[P1] Clear this execution-scoped context map when a prepared statement resets MVCC state. PREPARE and EXECUTE reuse the same StatementContext; resetMvccSnapshots clears normal pins and the preload completion marker, but not this map. The next EXECUTE reruns preload, skips buildContext here, and can retain prior external partitionItems plus freshness caches while its scan uses a fresh pin, allowing a stale MTMV rewrite. Clear/rebuild the map under the execution's exact pin and add PREPARE-to-first-EXECUTE and two-EXECUTE external-change tests.
There was a problem hiding this comment.
Fixed in 9487b21. StatementContext.resetMvccSnapshots now clears preloadedMtmvRefreshContexts together with the other execution-scoped snapshot state. StatementContextTest verifies a preloaded MTMV context is removed on reset.
| T result; | ||
| try { | ||
| return doAs(() -> action.call(pooled.client)); | ||
| result = doAs(() -> { |
There was a problem hiding this comment.
[P1] Install operation control before entering the outer Kerberos doAs. The action that starts HmsRemoteCallTracking.withTracker runs only after AuthAction.execute; Hive/Hudi authentication calls getUGI first, whose synchronized keytab first-use/refresh can block on another login or the KDC. KILL/deadline therefore has no watchdog here even though no wire call has started. Put this authenticated setup under the bounded control lifecycle (retaining the borrowed client until any abandoned auth task exits), recheck inside the callable, and add blocking Hive/Hudi keytab first-use/refresh tests.
There was a problem hiding this comment.
Resolved by scope reduction in 9487b21. The asynchronous/watchdog cancellation lifecycle around outer Kerberos authentication was removed. Authentication cancellation is no longer claimed and is explicitly outside this PR scope.
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: The HMS partition batching change had also introduced connector-wide query cancellation and deadline APIs. That lifecycle propagation substantially enlarged the change and remained incomplete across metadata entry points and blocking HMS client setup paths. Remove that SPI and lifecycle code so this change stays focused on the common HMS partition batching implementation. Keep bounded chunks, adaptive halving for size-limit failures, strict response integrity, cache single-flight loading, observability, and MTMV snapshot preload consistency. ### Release note Hive HMS partition batching no longer adds connector-level query cancellation or deadline APIs. Existing batching, fallback, integrity validation, cache sharing, and metadata access metrics remain unchanged. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run HmsPartitionBatchLoaderTest,CachingHmsClientTest,PreloadExternalMetadataTest,StatementContextTest,ConnectorSessionImplTest,PluginDrivenMvccExternalTableTest,HiveConnectorMetadataFileListStatsTest - ./run-fe-ut.sh --run HmsClientConfigBatchTest,ConnectorMetadataSurfaceTest,ConnectorPluginSurfaceTest,SummaryProfileTest,ConnectorMetadataAccessMetricsTest,MTMVPartitionUtilTest,MTMVRefreshContextBatchTest,MTMVRewriteUtilTest,MTMVTaskTest - ./build.sh --fe was attempted but stopped while downloading unixODBC because the downloaded archive was empty and failed MD5 validation - Behavior changed: Yes. Query cancellation and deadline propagation are removed from this PR; HMS partition batching behavior is retained. - Does this need documentation: No
There was a problem hiding this comment.
[P2] Separate logical partition access, batch execution, and HMS transport. A shared path for cache reuse, bounded batching, fallback, response validation, and metrics is the right direction. The problem is that the current implementation combines those responsibilities through HmsPartitionRequest and the HmsClient decorator chain, leaving several architecture issues:
HmsPartitionRequestis described as immutable request data, but it also carries aPartitionChunkConsumer, a shared mutableBatchExecutionState, and alogicalAccessOwnerflag. Its behavior therefore depends on which decorator copied it and which layer is currently executing it; it is both a request DTO and an execution context.- Cache publication is implemented as a reverse callback through the transport path.
CachingHmsClientinstalls the consumer, calls its delegate,ThriftHmsCliententersHmsPartitionBatchLoader, and the loader invokes the consumer to publish entries and complete owner work back inCachingHmsClient. This creates a logical dependency cycle between cache/access and transport/batching instead of a one-way pipeline. - The
HmsClient.getPartitions(HmsPartitionRequest)bridge now has an implicit behavioral contract that is stronger than its type contract.CachingHmsClientassumes that its delegate invokes the chunk consumer exactly once for every validated chunk, while the default implementation simply calls the legacy three-argument method and never invokes it. The cache consequently works correctly only with delegates that know this hidden decorator protocol, and the post-callunpublishedchecks are compensating for that abstraction leak. - There are two orchestration layers:
CachingHmsClientsplits misses into load windows and coordinates owners/waiters, whileHmsPartitionBatchLoadersplits those windows again and manages adaptive batch state. Logical and physical metrics then cross both layers through the shared request state. This is why cache configuration, batching, fallback, single-flight completion, partial publication, and metric aggregation have to be reasoned about together. - The transport is not a leaf abstraction.
ThriftHmsClient.getPartitions(request)owns the batch executor entry point rather than exposing a single rawgetPartitionsByNamesoperation, so replacing the transport or testing cache/invalidation behavior independently requires recreating the batching callback protocol.
Please keep the dependency direction one-way:
HmsPartitionAccess (cache lookup/publication, single-flight, invalidation fencing, admission, logical metrics) -> HmsPartitionBatchExecutor (chunking, halving, integrity validation, execution state, physical metrics, progressive delivery) -> HmsPartitionTransport (one getPartitionsByNames RPC).
The externally supplied request should contain only immutable logical inputs. Fallback size/timing and physical-event aggregation should live in an internal execution object owned by the batch executor. Progressive publication of fully validated chunks is useful and can remain, but pass that sink directly from the access orchestrator to the executor; do not carry it through HmsPartitionRequest and ThriftHmsClient.
This does not require removing the current cache, single-flight, fallback, or progressive-publication behavior. It changes their ownership boundaries so that each layer can be verified independently. Please preserve focused tests for all-hit and mixed hit/miss requests, overlapping owners/waiters, multi-window loads, a later-chunk failure after earlier chunks were published, and invalidation during a load. Those tests should verify that only the access layer publishes cache entries/completes owners, the executor alone aggregates physical attempts, and the transport performs only the requested single RPC.
TPC-H: Total hot run time: 17024 ms |
TPC-DS: Total hot run time: 83585 ms |
There was a problem hiding this comment.
[P2] Keep the MTMV integration as a narrow, engine-agnostic bulk-snapshot adapter. The MTMV requirement here is only to avoid calling the underlying related table once per mapped base partition. Before this change, MTMV already computes the complete mapping and then reads partition snapshots in per-partition loops. That can be batched without changing MTMV's lock, pin, planner, refresh, rewrite, display, and persistence lifecycles, and without any Hive/HMS branch in MTMV code.
The current implementation instead spreads preload ownership across MTMVRefreshContext, MTMVTask, PartitionsProcDir, MTMVRewriteUtil, StatementContext, and PreloadExternalMetadata, with separate lock/cached/cloud paths and execution-scoped context cleanup. This makes the Hive batching change responsible for new MTMV snapshot and lifecycle semantics; the review issues around preload ordering, persisted-set modes, cloud version loading, and prepared-statement reset are consequences of that enlarged boundary rather than requirements of HMS batching.
Please consider reducing this to the following generic adapter path:
MTMVRelatedTableIf#getPartitionSnapshotsshould be the only MTMV-facing abstraction. Give it a default loop overgetPartitionSnapshot, and let implementations that have an efficient bulk mechanism override it. MTMV should always call this bulk-shaped method; a separatesupportsPartitionSnapshotBatchLoadingbranch is unnecessary when the default already preserves compatibility and would leak an implementation/performance capability back into orchestration.PluginDrivenMvccExternalTableshould translate the generic MTMV snapshot request to connector freshness semantics. Hive may implement that connector call with HMS batching, while snapshot-id or local implementations use their own pinned/local representation.MTMVRefreshContextshould cache only genericMTMVRelatedTableIf -> partition name -> MTMVSnapshotIfvalues, never Hive/HMS objects, timestamps, batch sizes, or fallback state.- Given a set of MV partition names, the context can collect the union of mapped base-partition names per PCT table, call
getPartitionSnapshotsonce per table, validate the returned identities, and cache the result. Existing per-partition comparisons then read this cache. - Invoke that helper only where MTMV already owns the complete set: before the loops in
getMTMVNeedRefreshPartitions,isMTMVSync,getPartitionsUnSyncTables, andgeneratePartitionSnapshots; after rewrite's local grace/query/persisted-set filters; and, if necessary, once for allneedRefreshPartitionsbeforeMTMVTasksplits them into execution groups. - Keep the existing partition mapping,
baseVersions, MVCC-pin, locking, and persisted-snapshot semantics unchanged. With this shape, the newStatementContextpreload map, planner preload rule, lock-dependent context construction,refreshLocalState*variants, and task-level statement-context lifecycle are not needed for this feature.
This intentionally leaves any pre-existing external I/O-under-lock or cross-stage MTMV snapshot-consistency optimization out of scope; those can be addressed independently. The acceptance test for this PR can stay focused: a large external-table-backed MTMV operation should issue one logical bulk snapshot/freshness request, an HMS implementation may split it into bounded physical RPCs, first/changed mappings and later persistence should reuse the operation-local generic snapshot cache, and non-Hive MTMV behavior should remain unchanged.
ClickBench: Total hot run time: 14.89 s |
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
[P2] Split independent features from the bounded HMS batching change. The core behavior can be implemented and verified as one logical partition-name request entering a batch executor, which validates/reorders bounded responses and calls a one-RPC transport. The current PR additionally changes three largely independent policies, substantially increasing the proof and compatibility surface:
- Connector-wide telemetry.
ConnectorMetadataAccessEvent/Observer/Source,ConnectorContextandConnectorSessionpropagation, per-catalog metric ownership,SummaryProfile, and source tags in query/statistics/write callers are not required for those callers to receive batching: the existing three-argumentHmsClient#getPartitionscan enter the common loader transparently. Detailed source-labelled process/profile telemetry is useful, but it is a separate connector-framework feature with its own SPI and lifecycle contract. - Cache concurrency policy. Before this PR,
CachingHmsClientalready collected all misses and made one logical delegate call under an invalidation-generation fence. Bounded physical RPCs can therefore be added below the cache without introducing per-partition owners/waiters, FIFO admission, retry sharing, or progressive chunk publication. Those are valuable single-flight/capacity improvements, but they introduce independent concurrency and invalidation semantics and are the reason the cache now needs a reverse callback through the transport path. - Statistics sampling semantics. Selecting a small partition-name sample before materializing partition objects changes the statistics estimation algorithm and can reduce work from N objects to K objects; it is not the same optimization as splitting N objects into bounded RPCs. It should have its own before/after and estimation-compatibility validation rather than being coupled to the batch executor.
Please keep this PR focused on the pieces needed for safe batching: batch-size configuration, a one-way access/executor/transport boundary, strict missing/duplicate/unexpected/order validation, fallback only for explicit oversize failures, Hive/Hudi binding to the shared HMS client, and minimal cache miss reassembly/publication under the existing invalidation fence. Connector-wide observability, cache single-flight/progressive publication, and sample-before-materialization can then be reviewed independently. The MTMV caller can remain a thin engine-agnostic bulk-snapshot adapter as described in the separate MTMV review, without pulling planner/statement/lock lifecycle changes into this patch.
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Separate logical access, physical batch execution, and leaf HMS transport so cache publication is explicit and requests remain immutable. Preserve one adaptive execution across cache windows, let exact in-flight waiters attach without consuming an admission slot, bound new owners by the slot limiter, skip ineligible cloud MTMV preload candidates, and restrict size fallback to classified Thrift failures. ### Release note None ### Check List (For Author) - Test: Unit Test - 238 focused FE unit tests passed. - ./build.sh --fe was attempted but third-party setup downloaded an empty unixODBC archive and failed MD5 validation before FE build. - Behavior changed: Yes (HMS batching concurrency and cloud MTMV preload eligibility are corrected) - Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Large Hive partition requests need bounded HMS batches without duplicate concurrent loads, while MTMV rewrite must preload metadata without retaining unbounded statement state or performing external I/O under planner locks. Finalize the shared batch path with atomic per-table request registration, waiter-safe retry behavior, source-aware SPI compatibility, bounded cloud MTMV preload state, stable rewrite epochs, display-specific freshness checks, and stricter oversized-frame classification.
### Release note
Hive partition metadata access uses bounded batches, adaptive fallback, shared concurrent loads, and query-profile telemetry across query, MTMV, statistics, and display paths. Cloud MTMV preload now has a configurable snapshot budget and fails closed to the base plan when the budget is exceeded.
### Check List (For Author)
- Test: Unit Test
- 108 focused FE unit tests for HMS batching/cache concurrency, connector SPI compatibility, MTMV metadata preload, and statement context
- Full FE build was attempted but stopped before Maven because the unixODBC third-party archive download returned an empty file
- Behavior changed: Yes (HMS partition metadata requests are bounded and coalesced; cloud MTMV preload has a snapshot budget)
- Does this need documentation: No
|
/review |
|
run buildall |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: An interrupted active HMS partition-load owner could publish a nested InterruptedException that identical waiters treated as a shared remote failure. Detect interruption through the complete cause chain so waiters retry under their own operation context. Also freeze source-aware ConnectorMetadata overloads as default methods and fix the FE test import order required by Checkstyle.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- CachingHmsClientTest passed 38/38 in five consecutive focused runs
- ConnectorPluginSurfaceTest and PreloadExternalMetadataTest passed in the focused 47-test run
- Maven Checkstyle passed for the 60-module relevant FE reactor
- Behavior changed: Yes (waiters retry an active owner's local interruption instead of inheriting it)
- Does this need documentation: No
|
/review |
|
run buildall |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Round 2 converged with one remaining non-duplicate issue: cloud MTMV preload applies remote work and its all-or-nothing snapshot budget before USE_MV/NO_USE_MV selection, so excluded candidates can suppress the requested rewrite.
Checkpoint summary:
- Goal/tests: the batching, cache coordination, MTMV freshness, and observability changes are covered by focused production-path tests in the diff; I did not run builds or tests because this review harness prohibits execution.
- Scope/compatibility/config: all 70 FE paths were reviewed; the connector SPI surface and return types are frozen by baselines, and the new HMS/session settings validate at construction.
- Concurrency/lifecycle/observability: single-flight registration, invalidation, publication, slot release, context metric ownership, profile capture, and logical/physical event accounting are coherent at the reviewed head.
- Parallel paths/performance: scan, batch scan, write, statistics, Hive freshness, task/display refresh, and cloud rewrite paths were traced; batching/fallback and pre-lock metadata placement are otherwise bounded.
- Persistence/transactions/data writes/FE-BE: MTMV persistence uses the ungated union preload, Hive writes preserve exact partition actions, and this FE-only change introduces no BE variable or wire-format contract.
Existing live threads were treated as duplicate fences; no additional distinct issue survived the final sweep.
| ConnectContext connectContext = statementContext.getConnectContext(); | ||
| Map<MTMV, Set<MTMVRelatedTableIf>> eligibleMtmvs = new LinkedHashMap<>(); | ||
| Set<MTMVRelatedTableIf> pctTables = new LinkedHashSet<>(); | ||
| for (MTMV mtmv : statementContext.getCandidateMTMVs()) { |
There was a problem hiding this comment.
[P2] Apply USE_MV/NO_USE_MV before cloud preload and budget accounting. This collect-stage loop runs before Analyzer extracts those hints, while the actual hint filter is only applied later in getMaterializationContextByHint. Consequently bare NO_USE_MV and NO_USE_MV(A) still perform snapshot/version RPCs, and USE_MV(A) can lose A when A alone fits the limit but hinted-out candidates make this all-or-nothing union exceed it; the later cloud rewrite then rejects A because no context was preloaded. Share the same hint eligibility before collecting PCT tables/loading snapshots, and add zero-load NO_USE_MV plus filtered-budget USE_MV(A) tests.
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Large Hive partition loads need bounded HMS requests, reliable fallback and shared caller semantics. This follow-up isolates per-MTMV preload failures, avoids throwaway cloud partition mappings for pinned external PCT tables, applies MV hints before cloud preload budget accounting, guarantees replaced connector contexts are released even when connector close fails, and consolidates package-private HMS batch helpers without changing their behavior. ### Release note Hive HMS partition access uses bounded batches with adaptive fallback, shared cold-load coordination, strict result validation and profile/metric visibility. Cloud MTMV preload now respects USE_MV and NO_USE_MV before remote work and budget accounting. ### Check List (For Author) - Test: Unit Test and local synthetic performance test - 244 focused FE tests passed - 60-module FE Maven validate/checkstyle passed - Behavior changed: Yes (excluded MTMV candidates no longer consume cloud preload work or budget) - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: The shared HMS partition batch implementation needed three final lifecycle fixes. Nested USE_MV and NO_USE_MV hints inside subqueries, CTEs, and views were not available when cloud metadata preload ran; prepared statements could accumulate reparsed view hints across executions; background task contexts did not expose their final executor to connector Profile observers; and metrics keyed only by catalog name could collide while a dropped catalog and a same-name replacement overlapped. This change collects raw MV hints during relation collection and resets that generation before each top-level collection, binds final background executors to their contexts, and isolates metric ownership and labels by catalog ID plus name. It also keeps the total non-documentation addition at 6,000 lines by consolidating redundant tests and comments without changing batch behavior. ### Release note Hive HMS partition access now reports background-task metadata time consistently, handles nested MV hints without prepared-statement accumulation, and keeps metrics isolated across same-name catalog recreation. ### Check List (For Author) - Test: Unit Test and Maven validate - 244 focused FE tests from all 15 changed test classes passed - 74-module FE Maven validate/checkstyle passed with zero violations - 120,000-partition unit case verified 24 batches of 5,000 with complete ordered results - Behavior changed: Yes (nested MV hints participate in cloud preload selection and catalog metrics include catalog_id) - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Planner retries could reuse a connector statement scope that had already been closed by the previous attempt, retain stale MVCC rewrite state, or allow SET_VAR hints to re-enable cloud version caches during an E-230 recovery attempt. Background streaming and MTMV retries could also leave their ConnectContext installed on pooled worker threads. This change resets connector and MVCC state before every retry, keeps the two cloud version-cache TTLs disabled for the complete recovery attempt, clears stale MV rewrite partitions, and restores worker thread-local contexts with deterministic StatementContext closure. It also removes redundant test-only code so the total non-documentation addition remains below 6,000 lines. ### Release note Hive metadata retries now rebuild per-attempt connector state, and background task contexts are closed and restored deterministically. ### Check List (For Author) - Test: Unit Test and Maven validate - All 296 tests from the 17 changed FE test classes passed - The complete 74-module FE Maven validate/checkstyle reactor passed - Focused retry and context lifecycle tests passed - Behavior changed: Yes (retry attempts use fresh connector/MVCC state and E-230 recovery keeps cloud version caches disabled for the attempt) - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Connector metadata observers could create unbounded process-metric series by emitting distinct operation names. Statement-scope cleanup also stopped after a plugin resource threw Error, leaving later resources unclosed. Bound per-catalog operation cardinality with an overflow bucket and continue cleanup before rethrowing the first Error. ### Release note None ### Check List (For Author) - Test: Unit Test - 307 focused FE tests from all changed test classes - Full FE Maven validate / Checkstyle - Behavior changed: Yes (connector metadata metric cardinality is bounded and statement cleanup drains resources before rethrowing Error) - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: The bounded connector metadata metrics used a public-valid operation name for overflow aggregation, so a legitimate operation could be mixed with overflow traffic. An Error raised while closing the newly installed MTMV refresh statement scope could also bypass the task failure transition and retain its task context. Use a private collision-free metric label and route the MTMV cleanup failure through JobException after clearing both context references. Restore focused lifecycle coverage for both cases. ### Release note None ### Check List (For Author) - Test: - Unit Test: 307 tests across all 18 changed test classes passed - Maven validate: all 74 FE modules passed with zero Checkstyle violations - Behavior changed: Yes (metric overflow attribution and MTMV cleanup failure handling) - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Automated review status: complete and converged after three rounds on 41d91f39d5a2843631eee2e400abf7b70a3179eb. I found two distinct P2 observability/result-contract issues; both are attached inline.
Critical checkpoint conclusions:
- Goal and proof: the PR implements bounded/batched HMS partition loading, single-flight cache publication, source-aware metadata metrics, and pre-lock cloud-MTMV seed/pin handling. The added unit tests cover the main helpers, concurrency, freshness, hint, and reset behavior, but they do not prove the two production paths called out inline.
- Scope and clarity: broad, but the changes form a coherent connector-metadata/MTMV lifecycle update. The remaining gaps cross existing boundaries rather than being isolated helper mistakes.
- Concurrency: owner/waiter registration, invalidation generations, FIFO admission, interruption, failure publication, executor/thread-local cleanup, and MTMV lock ordering were traced. No new stale-write, deadlock, permit leak, or lock-order issue survived beyond existing review threads.
- Lifecycle/static initialization: connector metric references, context replacement/failure cleanup, statement scopes, background
ConnectContextrestoration, prepared execution, and retry resets were checked. No additional leak, premature release, circular lifecycle, or Java initialization problem was found; cross-TU C++ initialization is not applicable. - Configuration: catalog batch/fallback controls and the separate session cloud-preload budget are validated and consumed at their intended creation/execution scopes. No missing dynamic FE-process reload path was found; the independent cloud/generic controls expose the result-accounting issue inline.
- Compatibility: source-aware SPI defaults/forwarding and frozen surface baselines were reviewed. No new storage/wire-format or rolling-upgrade issue was found beyond already-existing compatibility threads.
- Parallel paths and conditions: all production partition-detail callers, Hive/Hudi, pooled/pool-disabled, cache-enabled/disabled, default/custom provider, QUERY/WRITE/STATISTICS/MTMV/DISPLAY, adaptive fallback, hint filters, excluded candidates, and retry paths were enumerated. The statistics-update lookup is the sole raw bypass and is reported inline.
- Tests and results: modified tests cover batching/order/integrity, concurrency/invalidation, metrics, snapshot generations, persistence, hints, prepared execution, and retry cleanup. End-to-end negative/profile tests for the two findings are missing. No tests or builds were run in this review environment, as required by the supplied review contract; no generated expected-result changes are present.
- Observability: logical/process/query aggregation and metric ownership were reviewed. The two remaining defects are precisely missing WRITE/retry telemetry and a false skipped/zero-time cloud-preload result.
- Transactions, persistence, and writes: commit/undo, MTMV persistence unions, master/task cleanup, local mapping/version refresh, and crash/error paths were traced. No new atomicity or persisted-state defect was found; the write-side defect is observability, not data mutation.
- FE/BE variables: no new variable requires FE-to-BE protocol propagation.
- Performance: batch sizing, adaptive degradation, single-flight overlap, admission limits, snapshot budgeting, and avoidance of remote work under FE locks were reviewed. No additional nonduplicate performance regression survived.
- Other/user focus: there was no additional user-provided focus. Every candidate is now accepted, dismissed with code evidence, or fenced by an existing live thread; Round 3 produced no new finding, so this review is not capped/incomplete.
| public List<Partition> getPartitionsByNames(String dbName, String tableName, List<String> partitionNames) | ||
| throws TException { | ||
| return HmsRemoteCallTracking.trackWireAttempt( | ||
| () -> super.getPartitionsByNames(dbName, tableName, partitionNames)); |
There was a problem hiding this comment.
[P2] Keep partition-statistics writes inside the new HMS access boundary. HiveConnectorTransaction.UpdateStatisticsTask reaches ThriftHmsClient.updatePartitionStatistics, whose direct client.getPartitionsByNames(...) call enters this override without HmsRemoteCallTracking.withTracker; CURRENT is therefore null and every RetryingMetaStoreClient attempt is invisible. These commit/undo lookups consequently emit neither a WRITE logical event/query-profile span nor physical retry counts, even though the parallel write-planning lookup is source-aware. Carry the session/WRITE source into statistics updates and route this singleton read through the same access/tracker boundary, with a production-chain retry-count test.
|
|
||
| private ExternalMetadataPreloadResult executePreload(StatementContext statementContext, List<Hint> hints) { | ||
| long preloadStartTime = TimeUtils.getStartTimeMs(); | ||
| preloadCloudMtmvRefreshContexts(statementContext, hints); |
There was a problem hiding this comment.
[P2] Report cloud-MTMV seed work as an executed preload stage. This call can load snapshots and install refresh contexts under the separate cloud snapshot budget, but the following generic table-preload gate can still return ExternalMetadataPreloadResult.skipped (notably with the default enable_preload_external_metadata=false). collectAndLockTable then records no preload timing and logs that preload was skipped even though remote work just completed. Compose the cloud and table outcomes so any performed work preserves the total elapsed time/executed state, and add a default-generic-off cloud test that asserts the returned result and planner profile.
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Hive tables with very large partition counts could either issue one metadata request per partition or send one unbounded getPartitionsByNames request. This change adds one common bounded HMS batch executor with strict response validation and adaptive fallback only for explicit oversized-request failures. It also adds a narrow MTMV bulk-snapshot adapter so mapped Hive partition freshness is loaded once per table and reused across refresh execution groups, while retaining the existing MTMV mapping, locking, and snapshot lifecycles.
### Release note
Hive Metastore partition-object requests are sent in configurable bounded batches, with adaptive fallback for explicit oversized-request failures.
### Check List (For Author)
- Test: Unit Test
- 111 focused FE core MTMV/MVCC tests passed
- 72 focused HMS, Hive, and connector SPI tests passed
- Maven validate passed across the relevant 60-module FE reactor with zero Checkstyle violations
- Behavior changed: Yes (large HMS partition-object requests are bounded and Hive-backed MTMV freshness uses bulk lookup)
- Does this need documentation: No
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
3 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Hive tables with very large partition counts could either issue one HMS partition-object RPC per partition on legacy caller paths or send every partition name in one unbounded
getPartitionsByNamesrequest. The first form creates excessive serial RPC latency; the second risks Thrift/HMS message limits and large temporary allocations.This PR narrows the change to the shared HMS partition-object boundary. Callers continue to submit one logical partition-name list through
HmsClient#getPartitions; the existing cache aggregates misses, one HMS batch executor owns bounded chunking, adaptive fallback and strict response validation, and a leaf transport performs onegetPartitionsByNamesinvocation per physical attempt. Query, statistics, write and Hive-backed MTMV callers therefore receive the same batching behavior without implementing their own chunk/retry loops.Common HMS batch execution
hive.hms_partitions_batch_size_per_rpcbounds each physical partition-object request; the default is 5,000.hive.hms_partitions_batch_fallback_timeout_msbounds the adaptive fallback phase; the default is 30,000 ms. It is checked between attempts and does not interrupt an active HMS RPC.hive.metastore.limit.partition.request/ “partitions scanned ... exceeds limit” failure is recognized.hive.metastore.client.pool.size=0, successful chunks in one logical request reuse one temporary HMS client. A failed physical call taints and destroys that client before a fallback attempt creates another.HmsClientConfig.Strict result integrity
Narrow MTMV bulk adapter
MTMVRelatedTableIf#getPartitionSnapshotshas a compatibility default that retains the existing scalar loop for non-bulk table implementations.HmsClient#getPartitionscall; the common executor then splits it into bounded physical requests.MTMVRefreshContextkeeps only a request-scoped table → partition → snapshot cache. It unions mapped base partitions before the existing loops in sync, need-refresh, display, persistence and rewrite paths.MTMVTaskpreloads the complete need-refresh union before splitting execution groups, so the default one-partition group size cannot regress first/manual/COMPLETE refreshes to singleton HMS requests.With the default batch size, a cold 120,000-partition logical object request becomes 24 bounded requests instead of one 120,000-name request. A 160,000-partition Hive-backed MTMV union becomes one logical bulk load and 32 bounded physical requests, rather than one object request per mapped partition.
Scope boundaries:
SplitSourcelifecycle behavior are unchanged.Release note
Hive Metastore partition-object access now uses configurable bounded RPC batches, strict response validation, and adaptive fallback for explicit oversized-request failures. Hive-backed MTMV partition freshness is aggregated into bulk logical requests before HMS batching.
Deterministic request-shape evidence
5000 → 2500 → 1250 → 625, then all objects completeThese rows describe deterministic orchestration and request shape; they are not a substitute for a real 120,000-partition HMS end-to-end rerun.
Validation
validatereactor passed with zero Checkstyle violations.git diff --checkpassed.thirdparty/installedis absent; no successful full./build.sh --ferun is claimed.