Skip to content

Fixes 32946: Replace entity repository inheritance with composition - #33248

Open
harshach wants to merge 14 commits into
mainfrom
harshach/split-entity-repo
Open

Fixes 32946: Replace entity repository inheritance with composition#33248
harshach wants to merge 14 commits into
mainfrom
harshach/split-entity-repo

Conversation

@harshach

@harshach harshach commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Describe your changes:

Fixes #32946

Replace the 13,569-line EntityRepository with 209 composed components and migrate repository, updater, service and application consumers while preserving REST contracts, Redis caching and transaction ownership.

Type of change:

  • Breaking change — Java extension interfaces

High-level design:

Seven startup assemblies bind Java 21 records, read/write services and ordered mutation policies around the retained DAO graph, preserving one owning transaction per existing flush, nested rollback, deadlock replay and deferred cache/search effects.

Java extensions migrate through the Collate companion, with the design, migration guide, verification status and performance evidence documenting the boundaries and coordinated rollout.

Open acceptance gates: repeatable API latency and the complete database/cache/width/load/allocation matrix, 90% coverage for every changed executable class, and CI on the current native and companion revisions

Tests:

Use cases covered

  • ID/FQN reads, requested fields, authorization, pagination, history, CSV and bulk operations
  • Single owning commits, enclosing rollback, deadlock replay and deferred Redis/search publication
  • One batched query for table and column metrics; exact extension scopes, overlapping column names, table-only selection and widths 3/100/1,000
  • Paginated columns without unused owner reads; persisted profiles, owner access and non-owner PII masking
  • RDF foreign-key projection and dedicated mapper inputs; suite configuration and named-graph isolation
  • Indexing-trigger completion before subsequent column-grid reads
  • Separate JVMs for secondary integration servers, preserving the primary DAO/transaction graph
  • Tour profiler permissions, pagination persistence and bounded RDF readiness recovery

Unit tests

  • Component and regression tests added
  • Metrics-stage service suite: 10,365 passes, one skip, no failures/errors
  • Final RDF/CSV selection: 890 passes; lifecycle default/retention selection: nine passes
  • New lineage graph/validation selection: ten passes; LineageHydrator line coverage rises from 4.26% to 97.16%
  • Native MCP: 630 passes
  • Browser shard planner: 114 passes; complete discovery fits 3,947 Chromium tests into 30 shards at the unchanged three-worker/19-minute budget; CI planning passes
  • Updated companion Slack selection: 628 passes; SlackComponents reaches 97.04% line coverage
  • Benchmark protocols: 43 passes, including real HTTP burst/authorization checks, failed warmup traces, measured allocation windows, concurrent cold-start reset and manifest upgrades
  • Portable benchmark/acceptance/coverage tools: 31 Python tests; real JaCoCo CLI fixtures pass for complete coverage and fail for missed nested classes; opt-in gates, not wired into CI workflows
  • Final companion service suite: 4,193 passes, six skips, no failures/errors; all 3,697 production class files match the package used for the modal API regressions; spec: 61 passes; MCP: 28 passes
  • Full native coverage gate: 349/1,115 executable classes below 90%, across 512 changed service/MCP sources; all 455 executable classes in the 209 extracted component sources reach 90%
  • Full companion coverage gate: 118/177 executable classes below 90%, across 105 changed service/plugin sources; both reports include only completed passing executions, with source/artifact hashes and no missing sources or class-identity warnings
  • Spotless and installed pre-commit checks passed on both branches

Backend integration tests

  • Real database/API regressions for transactions, cache publication, metrics, columns, RDF and indexing completion
  • PostgreSQL/MySQL with Redis: 79 critical-selection passes and one cache-mode assumption abort each
  • PostgreSQL/MySQL without Redis: 29 critical-selection passes and two cache-mode assumption aborts each
  • RDF: 115 passes; two pre-existing SQL access assertions excluded after reproducing on the original artifact
  • Application/column-grid regression selection: 44 passes on each database
  • Governance: 363 passes; search: 411 passes, with recorded configuration skips/assumption aborts
  • Ten lineage API classes: 184 passes and three skips on each database with Redis, no failures or assumption aborts
  • Additional 48-class consumer selection: 1,837 passes, 23 skips and 132 configuration assumption aborts per database, no failures
  • Companion modal and transaction/cache regressions: 20 passes per database; scoped results, full count, bounded page, statuses, soft deletion and incident rendering
  • Full canonical MySQL/Elasticsearch/Redis suite on ARM Java 21: 14,731 passes, zero failures, 101 skips and 1,247 configuration assumption aborts
  • Native isolated PostgreSQL/OpenSearch/Redis/RDF selection: 513 passes, zero failures, recorded skips/assumption aborts
  • Version-history empty terminal-page regression: three passes on each database; retains the last populated backward cursor after concurrent deletion
  • Companion canonical PostgreSQL/OpenSearch/Redis: 612 passes and three skips; isolated search: five passes; hybrid selection: 12 passes and two optional external/multi-node skips
  • Earlier broad suites and their failures retained with artifact provenance
  • All three native integration CI profiles, RDF, main/nightly browser and formatting checks passed at 49d9290397
  • Matching native 49d9290397 / Collate 4def2335b1: backend and governance/data access passed; all 37 applicable companion checks passed
  • Current native 1f52c5930b / Collate 8c0096662a: automatic CI pending; current-revision coordinated CI remains an open gate

Ingestion integration tests

  • Deterministic data-quality clock fixtures: 57 Python cases and midnight-rollover reproduction
  • Java ingestion/import consumers in backend selections

Playwright (UI) tests

  • Three production-bundle guided-tour entry flows, including profiler step 13
  • Three pagination persistence repetitions across table, Explore and Users pages
  • All 14 Knowledge Graph browser cases after an injected readiness HTTP 500
  • Full Playwright lint and changed-file formatting; recorded pre-existing TypeScript diagnostics

Performance validation

  • Frozen service 854c3d28…, original 306263df…; Java 21, fixed 1 GB G1 heaps, durable PostgreSQL 16/MySQL 8.3, OpenSearch 3.4 and Redis 7
  • 78 reads per database in warm/cold/L1-cold/unavailable/recovered Redis and Redis-disabled mode: 69 lower and nine equal SQL totals in every comparison; all 16 read/write comparisons pass across 10,320 measured responses
  • 60 mutation/CSV workloads per database: no higher synchronous SQL totals; Redis-disabled PostgreSQL has 53 lower and one equal, other measured write configurations have 54 lower; one owning commit per single-entity mutation
  • Five warm requests for 100-column table/column metrics: 505 → 5 SQL statements
  • Companion Slack 12-test modal fixture: 57 → at most six SQL statements on both databases, with unrelated table tests excluded
  • Earlier three five-pair warm latency series: 130,000 successful responses, with inconsistent column direction and unstable baseline p99
  • New Linux original-versus-original calibration: 100,000 measured responses, zero errors, five alternating pairs, fixed CPU/heap configuration; all six percentile repeatability groups fail, so no candidate latency acceptance
  • Acceptance driver: calibration before comparison, bracketed capacity, equal offered loads, overload/recovery and allocations after warmup; missing design workloads and incomplete matrix remain explicit failures
  • JFR confirms one shared request-cache serialization; isolated ETag allocation prototypes remain unadopted because results vary with fixture size
  • Repeatable acceptance commands, full class inventory and machine-readable checkpoint
  • Reviewable paired-run and SQL CSV summaries linked from the performance report; SQL/profiling timings excluded from latency acceptance

Manual testing performed

  • Local production bundle: open Help → Tour, advance through profiler step 13 and complete the flow
  • Local pagination checks: select table page size 25 and Explore page size 50, then verify Users page-size persistence
  • Isolated ARM64 Kind stack: reproduce MinIO setup failure and verify Argo artifact upload/download after the companion repair

UI screen recording / screenshots:

Production-bundle Help-entry guided tour, including profiler step 13

Checklist:

  • Issue linked; Java compatibility migration documented
  • Transaction/cache boundaries and validation provenance documented
  • Regression tests and captured UI recording
  • 90% coverage for every changed executable class
  • Complete API latency acceptance
  • Coordinated production implementation CI at 49d9290397 / 4def2335b1
  • CI on the current native and companion revisions

Native wrappers still dispatch Collate main, so both PRs require a coordinated rollout after the acceptance gates pass.

Compose Java 21 entity policies and shared read, mutation, history, bulk,
deletion and cache services, and migrate REST, MCP and background consumers.
Preserve owning transaction boundaries, retry replay and Redis publication
while batching metadata lookups and reusing canonical serialized rows.

Add regression suites, API benchmark protocols and architecture, extension
migration and performance documentation. Keep the unresolved latency and
changed-class coverage acceptance gates explicit.
@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Sep 12, 2026
@harshach
harshach marked this pull request as ready for review September 12, 2026 14:46
@harshach
harshach requested a review from a team as a code owner September 12, 2026 14:46
@greptile-apps

greptile-apps Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Too many files changed for review (904 files, 100 file limit).

Port upstream certification, history, column-lineage and Context Center
changes to the composed entity services. Retain owning transaction
boundaries and deferred cache/search effects.
@github-actions

github-actions Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 1f52c5930b647b27970d357f02a1045c154cc510 in Playwright run 34803141502, attempt 1.

✅ 4503 passed · ❌ 0 failed · 🟡 3 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 1h 5m 1s

⏱️ Max setup 5m 47s · max shard execution 24m 49s · max shard-job elapsed before upload 28m 11s · reporting 21s

🌐 218.89 requests/attempt · 2.24 app boots/UI scenario · 46.00% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 46% (convergence target: at most 15%).
  • Browser traffic was 218.89 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.24 per UI scenario (10673 boots / 4766 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard advanced-search-01 130 0 0 0 0 0
✅ Shard chromium-01 160 0 0 0 0 0
✅ Shard chromium-02 169 0 0 0 0 0
✅ Shard chromium-03 158 0 0 0 0 0
✅ Shard chromium-04 147 0 0 0 0 0
✅ Shard chromium-05 185 0 0 0 0 0
✅ Shard chromium-06 164 0 0 1 0 0
🟡 Shard chromium-07 238 0 1 0 0 0
✅ Shard chromium-08 166 0 0 0 0 0
✅ Shard chromium-09 162 0 0 0 0 0
✅ Shard chromium-10 204 0 0 0 0 0
✅ Shard chromium-11 158 0 0 0 0 0
✅ Shard chromium-12 159 0 0 0 0 0
✅ Shard chromium-13 212 0 0 0 0 0
✅ Shard chromium-14 193 0 0 0 0 0
✅ Shard chromium-15 199 0 0 0 0 0
🟡 Shard chromium-16 187 0 1 0 0 0
✅ Shard chromium-17 205 0 0 0 0 0
✅ Shard chromium-18 188 0 0 0 0 0
✅ Shard chromium-19 171 0 0 0 0 0
✅ Shard chromium-20 183 0 0 0 0 0
🟡 Shard chromium-21 172 0 1 0 0 0
✅ Shard chromium-22 165 0 0 0 0 0
✅ Shard data-asset-rules-01 65 0 0 0 0 0
✅ Shard domain-isolation-01 16 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard import-export-01 80 0 0 0 0 0
✅ Shard import-export-02 70 0 0 0 0 0
✅ Shard ingestion-01 46 0 0 0 0 0
✅ Shard ingestion-02 48 0 0 0 0 0
✅ Shard reindex-01 28 0 0 0 0 0
✅ Shard search-01 12 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0
🟡 3 flaky test(s) (passed on retry)
  • Pages/Tag.spec.tsVerify Owner Add Delete (shard chromium-07, 1 retry)
  • Features/ChangeSummaryBadge.spec.tsAI badge should appear on column description with Suggested source (shard chromium-16, 1 retry)
  • Pages/Lineage/DataAssetLineage.spec.tsverify create lineage for entity - File (shard chromium-21, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ UI Checkstyle passed — lint findings in changed files

🔍 ESLint findings in this PR's files — 0 error(s), 16 warning(s)

Errors block the build. Warnings do not yet — they are rules whose backlog is still
being worked down, listed so this PR does not add to it. See docs/ui-code-quality-gate.md.

0 error(s), 16 warning(s) across 1 changed file(s).

Count Rule
15 react-hooks/exhaustive-deps
1 openmetadata-imports/no-cross-page-imports
All findings
Location Rule Message
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:111:1 openmetadata-imports/no-cross-page-imports Page features must not import another page feature. Move shared code to components, hooks, interfaces, or pure utilities.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:168:6 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:217:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 't'. Either include it or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:442:6 react-hooks/exhaustive-deps React Hook useMemo has an unnecessary dependency: 'tableDetails.tags'. Either exclude it or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:466:6 react-hooks/exhaustive-deps React Hook useEffect has a missing dependency: 'setDqLineageData'. Either include it or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:512:9 react-hooks/exhaustive-deps The 'onTableUpdate' function makes the dependencies of useCallback Hook (at line 668) change on every render. To fix this, wrap the definition of 'onTableUpdate
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:512:9 react-hooks/exhaustive-deps The 'onTableUpdate' function makes the dependencies of useCallback Hook (at line 684) change on every render. To fix this, wrap the definition of 'onTableUpdate
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:539:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'onTableUpdate'. Either include it or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:553:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'onTableUpdate'. Either include it or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:613:6 react-hooks/exhaustive-deps React Hook useMemo has missing dependencies: 'customizedPage?.tabs' and 'feedCount'. Either include them or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:640:5 react-hooks/exhaustive-deps React Hook useMemo has a missing dependency: 'tabs'. Either include it or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:640:6 react-hooks/exhaustive-deps React Hook useMemo has a complex expression in the dependency array. Extract it to a separate variable so it can be statically checked.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:815:6 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'navigate'. Either include it or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:819:5 react-hooks/exhaustive-deps React Hook useCallback has a missing dependency: 'navigate'. Either include it or remove the dependency array.
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:891:6 react-hooks/exhaustive-deps React Hook useEffect has missing dependencies: 'fetchActivityCount', 'fetchTaskCounts', and 'setTableDetails'. Either include them or remove the dependency arra
🟡 src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:906:6 react-hooks/exhaustive-deps React Hook useEffect has missing dependencies: 'getTestCaseFailureCount' and 'tableDetails'. Either include them or remove the dependency array.

Fix locally (fast - only checks files changed in this branch):

make ui-checkstyle-changed

@github-actions

github-actions Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 72%
72.85% (102458/140636) 57.58% (61983/107634) 58.83% (20352/34590)

@gitar-bot

gitar-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Refactors entity repository from 13,569-line inheritance hierarchy to 209 composed components, migrating repository/updater/service policies across REST, MCP, search, RDF, CSV and workflow consumers while retaining Redis caching and reducing repeated metadata queries. Consider removing the redundant completeness check in searchForExport after the searchForCompleteExportResponse helper, since it already retries and guarantees a complete response or throws IOException.

💡 Quality: Redundant completeness check after export retry helper

📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchSearchManager.java:1491-1496 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchSearchManager.java:1531-1545 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchSearchManager.java:1419-1424

In both search managers, searchForExport calls the new searchForCompleteExportResponse(...) helper, which already retries and throws IOException on any timed-out/failed-shard response, so it only ever returns a complete response. The immediately following if (response.timedOut() || response.shards().failed() > 0) throw new IOException(...) block is therefore dead code. It's harmless but confusing; consider removing it so the completeness contract lives solely in the helper.

🤖 Prompt for agents
Code Review: Refactors entity repository from 13,569-line inheritance hierarchy to 209 composed components, migrating repository/updater/service policies across REST, MCP, search, RDF, CSV and workflow consumers while retaining Redis caching and reducing repeated metadata queries. Consider removing the redundant completeness check in `searchForExport` after the `searchForCompleteExportResponse` helper, since it already retries and guarantees a complete response or throws `IOException`.

1. 💡 Quality: Redundant completeness check after export retry helper
   Files: openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchSearchManager.java:1491-1496, openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchSearchManager.java:1531-1545, openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchSearchManager.java:1419-1424

   In both search managers, `searchForExport` calls the new `searchForCompleteExportResponse(...)` helper, which already retries and throws `IOException` on any timed-out/failed-shard response, so it only ever returns a complete response. The immediately following `if (response.timedOut() || response.shards().failed() > 0) throw new IOException(...)` block is therefore dead code. It's harmless but confusing; consider removing it so the completeness contract lives solely in the helper.

Options

Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@sonarqubecloud

Copy link
Copy Markdown

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend Backward-Incompatible-Change safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decompose the EntityRepository god class

2 participants