[IMPROVEMENT] DistributedRegistry Phase 2: tie registry lifetime to the engine context and settle the executor-side lookup API
- Dominant language
- Java
- Stars
- 6.2k
- Forks
- 2.5k
- Avg merge
- 2d 8h
- Merged PRs (30d)
- 111
Description
### Describe the problem
Follow-up to #19063, which scopes the process-wide `Registry.REGISTRY_MAP` / `HoodieSparkEngineContext.DISTRIBUTED_REGISTRY_MAP` refactor out of Phase 1 and lists "tying registry lifetime to `HoodieEngineContext` / write client rather than the JVM process" as a separate issue. #19575 (record index lookup counters) is the first consumer of the commit-boundary drain and deferred the items below here. Pointers are against #19575 head `0a9d4b412cae`.
### Status 2026-08-29
#19575 merged as `1112c37baf68`, and its final rounds did more of this than the checklist assumed. Sections A, B, D and E are done on master; only C is left.
The lookup registry is now an instance field, `HoodieSparkEngineContext.ownedRegistries` keyed by normalized base path (`RecordIndexLookupMetrics.registryKey`, no digest -- it "never leaves the context and never becomes part of a metric name"), resolved off the context via `getOrCreateOwnedRegistry` and drained by `removeOwnedRegistry`. It is never published into `Registry.REGISTRY_MAP`, so the `map`/`mapToPair`/`flatMap` closures do not carry it and the shutdown scrape cannot reach it. `ExecutorMetricRegistry.scopedName`, `removeMetricRegistry`, the stale-context and replacement branches, the `!(instanceof)` fallback, `ExecutorMetricsContext`, `NoOpRegistry` and the release interface default are all gone.
One thing this issue did not anticipate: the hardening layer left `DistributedRegistry.isRegisteredWith` / `registeredAppId` on master with **no callers**. The sites that still resolve out of the process-wide cache call bare `register()` -- `HoodieSparkEngineContext:286`, `SparkHoodieBackedTableMetadataWriter:124`, `SparkHoodieBackedTableMetadataWriterTableVersionSix:102` -- so #19063 gap 2 is still open and is being fixed by #19790. `isRegisteredWith` wants either wiring up or deleting.
### Checklist
**A. Registry lifetime (the refactor #19063 defers)**
- [x] Make the registry an instance field on `HoodieSparkEngineContext` keyed by base path, with `ExecutorMetrics.publish` reading it off `context` instead of `Registry.REGISTRY_MAP`. Removes the SHA-256 base-path digest in `ExecutorMetricRegistry.scopedName`, the stale-context and replacement branches in `getMetricRegistry`, and `removeMetricRegistry`. (#19575 https://github.com/apache/hudi/pull/19575#discussion_r3853269077) **Done in #19575** -- `ownedRegistries` + `getOrCreateOwnedRegistry` / `removeOwnedRegistry`; the digest, the stale-context branch, the replacement branch and `removeMetricRegistry` are all gone.
- [x] Keep executor-metric registries out of the `DISTRIBUTED_REGISTRY_MAP` capture in `HoodieSparkEngineContext.map/mapToPair/flatMap`: every job for every table in the JVM ships one accumulator per RLI table per task, and nothing evicts (40+ entries after one functional run). (https://github.com/apache/hudi/pull/19575#discussion_r3853269022) **Done in #19575** -- `ownedRegistries` is instance state and never enters that map, so the closures at L140-205 no longer carry it.
- [x] `Metrics.shutdown()` -> `registerHoodieCommonMetrics()` -> `Registry.getAllMetrics(true, true)` scrapes every registry in the process-wide map; the RLI registry's dotted name skips the common prefix, so leftover counters (abandoned commit, straggler task) are published as `..HoodieRecordIndexLookup.<12hex>.` and then cleared. Goes away once the registry is outside `REGISTRY_MAP`. (https://github.com/apache/hudi/pull/19575#discussion_r3853269153) **Done in #19575** -- the registry is never published into `Registry.REGISTRY_MAP`, so `getAllMetrics(true, true)` cannot reach it.
- [x] The `!(registry instanceof DistributedRegistry)` fallback in `getMetricRegistry` is uncovered (`TestDistributedRegistry` names it but never enters it); drop it with the map. (https://github.com/apache/hudi/pull/19575#discussion_r3853269168) **Done in #19575** -- `getMetricRegistry` is now `computeIfAbsent` + `register`, with no fallback branch.
**B. Executor-side lookup API (hudi-io surface added by #19575)**
- [x] Decide whether name resolution stays: `Registry.getRegistry(name)` via the `ExecutorMetricsContext` thread-local binding, `NoOpRegistry`, the `Registry.getRegistry` contract change, and the `HoodieSparkTable.getPreExecuteRunnable` binding. Both shipped emitters hold the bundle by closure, so passing the `Registry` in directly removes about 330 lines with identical behaviour. (https://github.com/apache/hudi/pull/19575#discussion_r3853269071, https://github.com/apache/hudi/pull/19575#discussion_r3817095063) **Done in #19575** -- it did not stay: `ExecutorMetricsContext` and `NoOpRegistry` are gone, `resolveRegistry` returns the registry and the lookup closures capture it.
- [x] `Registry.release` interface default and the `LocalRegistry` override are unreachable in production (only `ExecutorMetrics` calls `release`, always on a `DistributedRegistry`). Drop or test. (https://github.com/apache/hudi/pull/19575#discussion_r3853269157) **Done in #19575** -- removed; `release` lives on `DistributedRegistry` only.
**C. Cleanups**
- [ ] Delete the dead `HoodieMetadataMetrics.LOOKUP_RECORD_INDEX_*` constants (unused since `2a0d2aeef6a0` / HUDI-7391) and the `TODO [HUDI-9544]` in `HoodieBackedTableMetadata`, which #19575 supersedes. (https://github.com/apache/hudi/pull/19575#discussion_r3853269047)
- [ ] Caller attribution (`tag` vs `dedupe`) was removed from #19575 for lack of coverage; restore only with a DataSource `insert.drop.duplicates=true` test, if the dedupe path is worth it. (https://github.com/apache/hudi/pull/19575#discussion_r3853268998)
**D. Tests (from #19575, once A lands)**
- [x] Drop `TestRliLookupMetricsOnDataSourceMor` / `TestRliLookupMetricsOnDataSourceMorPartitioned` (22 writes; nothing under `RecordIndexLookupMetrics`, `ExecutorMetrics`, `DistributedRegistry` or the two lookup functions branches on table type, and both table types reach the drain through the same `commitStats`) and `TestRliLookupMetricsMultiTable` (separates gauges by `METRICS_REPORTER_PREFIX`, which is the table name, so it cannot exercise the digest; `TestRecordIndexMetricNames.countersAreScopedByBasePathNotOnlyByTableName` does). (https://github.com/apache/hudi/pull/19575#discussion_r3853269088) **Done in #19575** -- the MOR twins and `TestRliLookupMetricsMultiTable` are gone. What remains: `TestRliLookupMetricsOnDataSource`, `TestRliLookupMetricsOnSparkSql`, `TestRliLookupMetricsAcrossFailedCommit` and the unit `TestRecordIndexLookupMetrics`.
**E. Known limitation carried from #19575 (#19063 gap 3 on the new counters)**
- [x] An abandoned attempt's counters are carried into the next successful commit on every path that does not tear metrics down between writes: Spark SQL DML (`UpdateHoodieTableCommand`, `MergeIntoHoodieTableCommand`, `DeleteHoodieTableCommand` and `InsertIntoHoodieTableCommand` call `HoodieSparkSqlWriter.write` directly, bypassing `DefaultSource.cleanup()`) and `StreamSync` (only `close()` shuts metrics down). Fix: clear the group's registry at `startCommit` (as in rahil-c's `dd4a48c`), or on write-client construct/close per #19063 gap 3, then run `TestRliLookupMetricsAcrossFailedCommit` on the SQL path too. (https://github.com/apache/hudi/pull/19575#discussion_r3853268971) **Done in #19575** -- `resolveRegistry` clears at lookup construction (`RecordIndexLookupMetrics:92`), which is the `dd4a48c` fix. danny0405's caveat still stands (#19575 r3870639302): the clear is shared by write clients that share one `HoodieSparkEngineContext`.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.