Fixes #26674: improve PATCH table API performance - deduplicate lineage ops and optimize column matching #26848
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
khoaihps
force-pushed
the
khoaihps-fix/improve-patch-table-api
branch
from
394e4aa to
008a02a
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
Pull request overview
Improves performance of PATCH /api/v1/tables/{id} by reducing redundant lineage search updates, avoiding expensive search refresh blocking on column-lineage cleanup, and optimizing column matching during entity updates.
Changes:
- Deduplicates deferred column-lineage search updates in
TableRepositoryand targetstable_search_indexinstead of the global"all"alias. - Sets
refresh=falseon upstream-lineage column deletionupdateByQuerycalls for both Elasticsearch and OpenSearch clients. - Optimizes
ColumnEntityUpdater.updateColumns()matching by using a prebuilt lookup map instead of an O(n²) stream search.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchEntityManager.java | Disables blocking shard refresh for column-lineage deletion update-by-query. |
| openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchEntityManager.java | Disables blocking shard refresh for column-lineage deletion update-by-query. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java | Accumulates and flushes column-lineage search updates once; switches lineage ops to table_search_index. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Replaces per-column stream search with O(n) lookup for matching stored vs updated columns. |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
khoaihps
force-pushed
the
khoaihps-fix/improve-patch-table-api
branch
from
f206363 to
ea7b5db
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
khoaihps
force-pushed
the
khoaihps-fix/improve-patch-table-api
branch
from
dcb19ae to
959de76
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
| // unless configured otherwise) | ||
| // instead | ||
| // of forcing a blocking shard refresh after each updateByQuery. Lineage | ||
| // cleanup does not require immediate read-after-write consistency. |
There was a problem hiding this comment.
Same comment formatting issue here: the refresh=false rationale is broken across lines with a standalone // instead. Please reflow for readability/consistency.
| // unless configured otherwise) | |
| // instead | |
| // of forcing a blocking shard refresh after each updateByQuery. Lineage | |
| // cleanup does not require immediate read-after-write consistency. | |
| // unless configured otherwise) instead of forcing a blocking shard refresh | |
| // after each updateByQuery, since lineage cleanup does not require immediate | |
| // read-after-write consistency. |
| // after | ||
| // each | ||
| // updateByQuery. Lineage cleanup does not require immediate read-after-write | ||
| // consistency. |
There was a problem hiding this comment.
The inline comment above .refresh(Refresh.False) is broken into multiple single-word lines (e.g., // after, // each, // updateByQuery), which makes it hard to read and doesn’t match typical formatting in this file. Please reflow it into normal wrapped lines.
| // after | |
| // each | |
| // updateByQuery. Lineage cleanup does not require immediate read-after-write | |
| // consistency. | |
| // after each updateByQuery. Lineage cleanup does not require immediate | |
| // read-after-write consistency. |
| .refresh(Refresh.False)); | ||
| // refresh=false: rely on the index's default refresh interval (default 1s unless configured | ||
| // otherwise) instead | ||
| // of forcing a blocking shard refresh after each updateByQuery. Lineage | ||
| // cleanup does not require immediate read-after-write consistency. |
There was a problem hiding this comment.
The refresh=false rationale comment is placed after the updateByQuery(...).refresh(Refresh.False)); statement and is awkwardly line-wrapped (splits phrases like "otherwise) instead" / "Lineage"). Consider moving the comment immediately above the .refresh(...) call and reflowing it for readability.
| .refresh(Refresh.False)); | |
| // refresh=false: rely on the index's default refresh interval (default 1s unless configured | |
| // otherwise) instead | |
| // of forcing a blocking shard refresh after each updateByQuery. Lineage | |
| // cleanup does not require immediate read-after-write consistency. | |
| // refresh=false: rely on the index's default refresh interval (default 1s | |
| // unless configured otherwise) instead of forcing a blocking shard refresh | |
| // after each updateByQuery. Lineage cleanup does not require immediate | |
| // read-after-write consistency. | |
| .refresh(Refresh.False)); |
| // unless configured otherwise) | ||
| // instead | ||
| // of forcing a blocking shard refresh after each updateByQuery. Lineage | ||
| // cleanup does not require immediate read-after-write consistency. |
There was a problem hiding this comment.
The refresh=false rationale comment is oddly split across lines (standalone // instead) which hurts readability. Please reflow it into normally wrapped lines to match the surrounding style.
| // unless configured otherwise) | |
| // instead | |
| // of forcing a blocking shard refresh after each updateByQuery. Lineage | |
| // cleanup does not require immediate read-after-write consistency. | |
| // unless configured otherwise) instead of forcing a blocking shard refresh | |
| // after each updateByQuery. Lineage cleanup does not require immediate | |
| // read-after-write consistency. |
harshach
added
the
safe to test
Code Review ✅ Approved 5 resolved / 5 findingsImproves PATCH table API performance by deduplicating lineage operations and optimizing column matching, resolving five issues including blocking refresh calls, stale entry accumulation, and duplicate column key handling. No issues remain. ✅ 5 resolved✅ Performance: updateColumnsInUpstreamLineage still uses blocking refresh(true)
✅ Bug: pendingDeletedColumnFqns accumulates stale entries across consolidation passes
✅ Performance: Reverts GLOBAL_SEARCH_ALIAS→TABLE_SEARCH_INDEX optimization
✅ Bug: OpenSearch deleteColumnsInUpstreamLineage still uses Refresh.True
✅ Edge Case: Duplicate ColumnKey when origColumns has case-variant names
OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
🔴 Playwright Results — 2 failure(s), 20 flaky✅ 2957 passed · ❌ 2 failed · 🟡 20 flaky · ⏭️ 174 skipped
Genuine Failures (failed on all attempts)❌
|
Describe your changes:
Fixes #26674
The
PATCH /api/v1/tables/{id}endpoint was taking 49–99 seconds per request during Database Service ingestion workflows. Profiling showed ~99% of time spent in internal processing, with lineage-related OpenSearch operations executing 2–4× per request and blocking until all shards refreshed.Three root causes were identified:
1. Duplicate deferred lineage operations
When
consolidateChanges=true,updateInternal()runs multiple times within a single request. Each run was independently callingdeferReactOperation()for column rename/delete lineage cleanup, resulting in the sameupdateByQuerybeing fired 2–4× against 13,000+ documents per request.2. Blocking index refresh on lineage
updateByQuerycallsBoth Elasticsearch and OpenSearch
updateByQuerycalls usedrefresh=true, which forces a shard refresh and blocks the request thread until all shards complete. With 13,275+ documents being updated, this caused ~90-second stalls per operation. Since thetable_search_indexrelies on the defaultindex.refresh_interval(1 second), settingrefresh=falseallows the engine to apply changes asynchronously within the next refresh cycle — which is acceptable for lineage cleanup, as there is no requirement for immediate read-after-write consistency here.3. Lineage operations scanned the entire cluster
updateColumnsInUpstreamLineageanddeleteColumnsInUpstreamLineagewere usingGLOBAL_SEARCH_ALIAS("all") — an alias that spans every index in the cluster (~30+ indices). Column-level lineage (upstreamLineage.columns) is only stored in 8 entity indices:table,topic,container,dashboard_data_model,search_entity,api_endpoint,mlmodel, anddashboard. RunningupdateByQueryagainst all indices was scanning irrelevant shards, wasting I/O and cluster resources.Bonus: O(n²) column matching
ColumnEntityUpdaterwas callingorigColumns.stream().filter(columnMatch).findAny()inside a loop overupdatedColumns, resulting in O(n²) comparisons on wide tables.What
TableRepository.java— Replace per-calldeferReactOperation()with a single deferred flush viaflushPendingColumnLineageSearchUpdates().entitySpecificUpdate()clearspendingDeletedColumnFqns/pendingRenameColumnFqnsonce perupdateInternal()pass;handleColumnLineageUpdates()accumulates into them across all recursive column-level calls. AcolumnLineageUpdateDeferredboolean ensures the flush is registered exactly once regardless of how many passes consolidation runs.SearchClient.java— AddCOLUMN_LINEAGE_SEARCH_INDICESconstant: a comma-separated list of the 8 indices that storeupstreamLineage.columns.SearchRepository.getIndexOrAliasName()already handles comma-separated index names, so no lower-level changes are needed.TableRepository.java— Switch fromGLOBAL_SEARCH_ALIAS("all") toCOLUMN_LINEAGE_SEARCH_INDICESfor all column lineage search operations, targeting only the indices that can contain column lineage data.ElasticSearchEntityManager.java/OpenSearchEntityManager.java— Changerefresh=true→refresh=falseon bothupdateColumnsInUpstreamLineageanddeleteColumnsInUpstreamLineage. Thetable_search_indexhas no explicitindex.refresh_intervalconfigured, so it uses the OpenSearch/Elasticsearch default of 1 second. Column lineage cleanup changes will be visible within that window, which is sufficient — no caller requires immediate post-PATCH search consistency for renamed or deleted columns.EntityRepository.java— Replace O(n²) column matching with aHashMap<ColumnKey, Column>lookup, whereColumnKeyis arecord(name, dataType, arrayDataType). Using a record avoids delimiter-collision bugs that would arise from string concatenation, and reduces matching from O(n²) to O(n).How to test
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>test_deletedColumnLineagePropagatesInSearchinTableResourceITverifies that after a column is removed from a table, the column lineage reference is cleaned up in the search index within the default refresh interval.