[improvement](hive) Batch Hive metastore partition access - #67186
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
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: The latest master added a TIMESTAMP_NS rollup test that still called the pre-change MTMV partition-column and range-rollup signatures. After CI rebased the PR, fe-core test compilation failed. Pass Optional.empty() through both calls so the new master test follows the snapshot-aware API introduced by this PR. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.mtmv.MTMVRelatedPartitionDescRollUpGeneratorTest,org.apache.doris.mtmv.MTMVTaskTest,org.apache.doris.connector.spi.ConnectorMetadataFreshnessDefaultsTest,org.apache.doris.connector.hive.HiveConnectorMetadataSiblingDelegationTest - Behavior changed: No - Does this need documentation: No
|
run buildall |
|
run performance |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16872 ms |
TPC-DS: Total hot run time: 81937 ms |
ClickBench: Total hot run time: 14.66 s |
FE Regression Coverage ReportIncrement line coverage |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
…on PRs from master in merge order (#67753 #67802 #67814 #67837 #67853 #67876) (#68017) Cherry-picked from #67753, #67802, #67814, #67837, #67853, #67876 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830. | # | Master commit | PR | Title | |---|---|---|---| | 1 | fe39f5b | #67753 | [fix](ivm) Answer FE-computable dry runs on the frontend instead of a placeholder backend | | 2 | f8ed33f | #67802 | [fix](ivm) Refresh the surviving partitions after an IVM baseline rebuild | | 3 | 7bd89a0 | #67814 | [fix](ivm) Stop the incremental delta from reading partitions the MV dropped | | 4 | 3050a9a | #67837 | [fix](ivm) Invalidate the baseline when a column used by the MV is dropped | | 5 | 22c95eb | #67853 | [fix](ivm) Carry the row-binlog hidden columns in the analyzed MTMV schema | | 6 | 3390a7a | #67876 | [test](ivm) Remove unnecessary cloud skips from IVM suites | Not included on purpose: - The 11 labelled PRs that already carry `incremental-computation-picked` (#62606 in the fork point, #67508 via #67712, the nine of #67830). - #67820 is still open on master; this branch already carries its content via #67861. ### Prerequisite check None of the six PRs declares a prerequisite, and none of them needs another master PR for its behavior. The only master commits that touch the same files and are not on this branch are unrelated to incremental computation (#66761 TIMESTAMP_NS, #67545 DLF, #67569 / #67520 / #67835 MySQL-protocol and session refactors, #67186 Hive partition batching, #67787 SQL cache user variables); they were left out, and two picks needed a mechanical adaptation because of that: - **#67753** conflicted in `StmtExecutor.sendMetaData`: master had already extracted the post-metadata EOF into `sendMetadataTerminatorIfNeeded(channel)` (#67520, a Connector/J cursor-fetch fix). The branch keeps its inline EOF block and now sends it on the given `channel` instead of `context.getMysqlChannel()`, which is exactly what the extracted helper does on master. Everything else in the pick is identical to the master commit. - **#67814** applied cleanly but did not compile: the new `MTMVPartitionUtil.generateRelatedBasePartitionIds()` returns an `Optional`, and on master `import java.util.Optional;` came with #67186. The import was added to the pick commit; that is the only difference from the master commit. The other four picks applied without conflicts and are byte-identical to their master commits (diffs compared with `index`/`@@` lines stripped). Both adaptations are recorded in the respective commit messages. ### Drift check against master After the six picks, every touched file is byte-identical to master at `3390a7a721f` except `MTMV.java`, `MTMVTask.java`, `MTMVPartitionUtil.java`, `MTMVPartitionUtilTest.java`, `MTMVTaskTest.java` (differences = #67186 + #67545 + #66761), `CreateTableInfo.java` (= #67787) and `StmtExecutor.java` (= #67520 + #67569 + #66761 + the later session refactors #67835 / #67883 + this branch's #67861). For the first six files, applying those unrelated master commits on top of the branch's versions reproduces master's files exactly; for `StmtExecutor.java`, the diff against master right after #67753 (`fe39f5b6a42`) consists only of #67520 / #67569 / #66761 / #67861 hunks. So nothing IVM-related is missing. The regression framework, plugins and the whole `mtmv_p0/ivm` suite/data directories are identical to master. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift, compiles fe-core main + test) with the 17 test classes touched by the picks or extending the touched `IvmDeltaTestBase`: 17 classes, 404 tests, 0 failures, 0 errors, BUILD SUCCESS (5:19 min) — `MTMVPlanUtilTest` 24, `IvmAggDeltaHandlerTest` 33, `IvmDeltaRewriteHelperTest` 17, `IvmNormalizeMTMVJoinTest` 44, `IvmJoinDeltaHandlerTest` 23, `IvmDeltaRewriteStateTest` 10, `IvmPlanSignatureGeneratorTest` 22, `IvmBaselineRebuildTest` 28, `IvmLinearDeltaHandlerTest` 39, `IvmDeltaRewriterTest` 23, `IvmNormalizeMTMVUnionTest` 10, `MTMVTaskTest` 50, `MTMVPropertyUtilTest` 13, `MTMVPartitionUtilTest` 16, `SchemaChangeHandlerTest` 22, `StmtExecutorInternalQueryTest` 3, `StmtExecutorTest` 27. - FE checkstyle on fe-core: 0 violations. - No BE, cloud or thrift changes in this batch. - All 18 touched groovy files (framework `Suite.groovy`, `plugin_planner.groovy`, 16 suites) parse cleanly (groovy parser check). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: yujun <yujun@selectdb.com>
yiguolei
added
dev/4.1.5-merged
and removed
dev/4.1.x
dev/4.1.x-conflict
dev/4.2.x
labels
…on PRs from master in merge order (#67347 #68193 #68336 #68170 #68180) (#68405) Cherry-picked from #67347, #68193, #68336, #68170, #68180 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each ending with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017, #68073, #68151, #68236 and #68303. | # | Master commit | PR | Title | |---|---|---|---| | 1 | a7c0a78 | #67347 | [fix](cloud) bind the packed slice location lifetime to its writer | | 2 | addf0c8 | #68193 | [feature](ivm) Add the per-partition refresh state and its journal channel | | 3 | e885b46 | #68336 | [refactor](ivm) Rename IvmInfo.refreshVersion to sequencePrefix | | 4 | 01efbca | #68170 | [fix](ivm) Fall back to complete refresh when the IVM stream is unusable | | 5 | d6c1a2b | #68180 | [fix](ivm) Choose IVM baseline rebuild partitions from the MV partition mapping | Not included on purpose: - The 29 labelled PRs that already carry `incremental-computation-picked` (every other closed PR with the label). This batch closes the selection query: 34 closed labelled PRs = 29 already picked + these 5. ### Prerequisite check - **The IVM series (#68193, #68336, #68170, #68180)** — one series by the same author, merged 2026-09-21/22, all tracing to issue #65418. #68193 says in its own description that it "adds the state that the following PRs need" — `MTMVPartitionState` plus its journal channel — and #68336 is the rename of the field sitting next to it. The PRs that consume that state are in this batch too, so the series is carried whole and in master merge order. Its earlier PRs (#67802, #67837, #67814, #68138, #67646, #67669, #67575) were picked in the previous rounds. - **#67347** is standalone (cloud packed-file slice lifetime). Every file that uses the APIs it changes is in the pick; `be/src/io/fs/packed_file_system.cpp`, which also reads the global slice index but is not modified, compiles unchanged against the picked headers. - **No unpicked master commit is required by any of the five.** Verified by compiling, not by inspection alone: the whole FE main + test tree compiles and the touched unit tests pass (below), and every BE/cloud file the picks touch passes a `-fsyntax-only` compile with the real build's flags. - The unlabelled master commits touching the same files (#67186 Hive partition batching, #66530 external scan task reuse, #66761 TIMESTAMP_NS, #67545 DLF, #67067 eager-agg) are *not* prerequisites — see the drift check: none of the picks' own added lines reference what they introduce. ### Drift check against master Per pick, `git show` of the branch commit against the master squash, with `index`/`@@` lines stripped: #67347, #68193 and #68336 are identical. Two differ, both mechanically: - **#68170 / `MTMVTask.java`** — master's context around `executePartitionBasedRefresh` carries #67186's `try` block and snapshot preload; this branch does not have #67186, so the auto-merge kept the branch's block. The pick's own added and removed lines are identical to master's. - **#68180 / `MTMV.java`** — master's squash also adds `import org.apache.doris.datasource.mvcc.MvccSnapshot;`, for #67186's `pinnedSnapshots` overloads of `calculatePartitionMappings` / `getEffectiveQueryUsedBaseTablePartitionMap`. Those overloads do not exist here and the pick's new code only calls the single-argument form, which exists on both sides, so the import is not needed and not carried. For every file this batch touches, `git diff upstream-apache/master -- <file>` was taken and each master-side line attributed to the unpicked commit that added it. All 42 files resolve: 37 have no master-side difference at all, and the 5 that do are fully owned by | file | master-side lines | owner | |---|---|---| | `MTMV.java` | 20 | #67186 | | `MTMVTask.java` | 46 | #67186, #66530 | | `MTMVTaskTest.java` | 44 | #67186 | | `MTMVPartitionUtil.java` | 65 | #67186, #67545 | | `MTMVRelatedPartitionDescSyncLimitGenerator.java` | 3 | #66761 | i.e. nothing belonging to the picks is missing, and no unlabelled commit has to come along. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main 4480 files + test 1485 files) over the 11 touched test classes — `AlterMTMVTest` 25, `MTMVTest` 23, `MTMVTaskTest` 48, `MetaLockUtilsTest` 6, `IvmBaselineRebuildTest` 37, `IvmAggDeltaHandlerTest` 33, `IvmDeltaRewriteStateTest` 10, `IvmFailureReasonTest` 1, `IvmInfoTest` 6, `IvmSequenceCalculatorTest` 4, `DatabaseTransactionMgrTest` 20 — **213 tests, 0 failures, 0 errors, 0 skipped, BUILD SUCCESS**. - FE checkstyle on fe-core: 0 violations. - BE/cloud: `-fsyntax-only` with the Release flags of the real build and its own compiler (`/opt/homebrew/opt/llvm@20/bin/clang++`) on `be/src/io/fs/packed_file_manager.cpp`, `be/src/io/fs/packed_file_writer.cpp`, `be/src/cloud/cloud_rowset_writer.cpp` and `be/test/io/fs/packed_file_manager_test.cpp` (the last with `-DBE_TEST -fno-access-control`): no errors. - The three new regression suites parse (`test_ivm_baseline_marker_scope`, `test_ivm_chained_stream_scope`, `test_ivm_partitions_fallback_stream_unusable`); their `.out` files are the upstream ones, unmodified. --------- Co-authored-by: Xin Liao <liaoxin@selectdb.com> Co-authored-by: yujun <yujun@selectdb.com>
5 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.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.
Query Profile observability
Connector Metadata Accessprofile through the existingConnectorScanProfilehook.HmsPartitionRequest.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. Hive Query Profile also shows the resulting partition-batch request shape and elapsed time.
Deterministic request-shape evidence
5000 → 2500 → 1250 → 625, then all objects completeThese rows describe deterministic orchestration and request shape. Real HMS measurements follow.
Real HMS performance evidence
Both measurements used a Hive 2.3.2 Metastore backed by PostgreSQL 9.5.3 over loopback Thrift TCP. The partition cache was excluded from the timed A/B reads, returned counts and checksums matched, and the default physical batch size was 5,000.
The large-scale transport benchmark reproduced the exact before/after HMS API shapes against 120,000 real partitions. The before side issued one
getPartitionsByNames(singleton)call per partition; the after side issued boundedgetPartitionsByNamescalls:A separate small-scale run used freshly compiled current-head (
cd5db406e18cb5a7808e6c8b393d82fb82684d74) Doris production classes. Class-load tracing confirmed the pathCachingHmsClient -> ThriftHmsClient -> HmsPartitionBatchExecutor -> real HMS/PostgreSQL. Both sides preconstructed the same names and warmed the connection before timing; the before side reproduced the singleton Doris call shape and the after side made one logical bulk call:The 120,000-partition run isolates the real HMS transport/database bottleneck and does not include Doris conversion. The current-head run includes Doris conversion, cache lookup, identity parsing, strict reordering and batch execution, but directly invokes the production classes rather than running a full FE/BE SQL or MTMV refresh. End-to-end improvement therefore still depends on the share of refresh/query latency originally spent in HMS metadata access.
Validation
Latest review increment: 13 HMS batch-executor tests and 20 PluginDriven scan batch/profile tests passed; Hive/Hudi catalog-property tests also passed.
111 focused FE-core tests passed: MTMV refresh context, partition utilities, rewrite, task, and plugin-driven MVCC table paths.
72 focused connector tests passed: HMS batching/cache/Thrift integration, Hive freshness, and connector SPI surface.
The final no-cache 60-module Maven
validatereactor passed with zero Checkstyle violations.git diff --checkpassed.Effective PR diff against its master base: 43 files, 3,086 additions and 207 deletions, excluding the uncommitted design/review documents.
Three independent final review scopes converged with no new P1/P2 findings after fixing task preloading, pool-disabled client reuse, and Hive's standard partition-limit classifier.
The standard targeted FE test runner compiled the current-head 60-module reactor successfully after the worktree's standard prebuilt third-party package was restored.
HmsPartitionBatchExecutorTestran 13 tests with no failures or errors.Full Doris MTMV refresh version A/B
A real binary-version A/B ran
REFRESH MATERIALIZED VIEW ... COMPLETEagainst a Hive table with 100 partition metadata rows and empty S3 prefixes in local MinIO. The before side used the released Doris 4.1.3-rc02 FE (7126cf65d96); the after side used this PR’s FE (cd5db406e18cb5a7808e6c8b393d82fb82684d74). Both sides used the same running Doris 4.1.3-rc02 BE, Hive Metastore, PostgreSQL, MinIO data, catalog properties, and MV definition. To preserve the exact MV and cluster state, current-head FE started from a copy of the measured 4.1.3 FE metadata and upgraded it in place.Both Doris partition caches were disabled. Each FE version had one excluded warm-up followed by five serialized measured COMPLETE refreshes.
7126cf65d96)get_partitioncd5db406e18)getPartitionsByNamesThis is a measured 4.01x full-refresh speedup, 75.05% mean latency reduction, and 100x physical RPC reduction (99%). The three remaining batched calls are the refresh path’s three independent partition-metadata stages; each stage loads all 100 objects in one physical request, whereas 4.1.3 issues 100 scalar requests per stage. Durations are FE task-start to
MTMVService.refreshCompletetimestamps; RPC counts and method names come from the Hive Metastore log.Environment: same running Doris 4.1.3-rc02 BE (
7126cf65d96) for both sides; Hive Metastore 2.3.2 + PostgreSQL 9.5.3 + MinIO on the same host. Empty partition data deliberately isolates metadata/MV orchestration from file-scan cost, so this is a real end-to-end MTMV refresh version A/B but not a representative data-scan benchmark.Scaling note: with
Npartitions, Doris 4.1.3 performs approximately3Nremote partition-object calls in this refresh path. This PR performs approximately3 × ceil(N / batchSize)calls (the default batch size is 5,000). Both versions still read, deserialize, and processNpartition objects, so total work retains an O(N) component; the improvement removes the per-partition network round trips rather than making refresh time constant. The speedup therefore generally grows with partition count until HMS serialization, Doris object processing, or MV partition work becomes dominant.