Fixes #33133: optimize java-playwright-nightly - #33134
Draft
mohityadav766 wants to merge 8 commits into
Draft
Conversation
Twenty-three of the 34 *UIIT.java classes document themselves as "Java port of <spec>.ts", and every spec they name exists in the UI suite. They asserted the same UI behaviour a second time, in a slower harness, and cost 38.8 of the 58.4 minutes the ui-it matrix spent running tests. The one thing the ports added was a reindexEntities call between UI mutations. That is already an established pattern in the UI suite -- TestCaseStatusAfterReindex, TestSuiteListAfterReindex and TestSuiteSummaryAfterReindex each inject one against a real regression -- so it is not a reason to keep a parallel Java copy of the whole DataQuality/IncidentManager/Profiler surface. Kept: the eight search-indexing UIITs with no UI equivalent, and GoogleSsoSignInUIIT (skipped unless jpw.auth=sso-google-confidential, so it costs nothing and shares the mock-IdP harness with the two backend SSO tests). The only assertion with no home in the UI suite was "column profile charts still render after a reindex" -- Profiler.spec.ts already asserts the four graphs, but not post-reindex. Ported to ColumnProfileGraphsAfterReindex.spec.ts. Also deletes the eight page objects left with no referrer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…able
A recursive hard delete of any service with an ingestion pipeline underneath it
aborted outright whenever the pipeline service client could not reach Airflow:
IngestionPipelineRepository.postDelete -> deleteDeployedPipeline
-> AirflowRESTClient.deletePipeline
-> PipelineServiceClientException: Unable to connect to Airflow APIs
The exception propagated out of the cascade and rolled the subtree back, so the
service survived. The async endpoint had already answered 202, which left the
failure visible only in a server-side ERROR line -- callers just saw the entity
never disappear.
deleteDeployedPipeline already had a tolerance path, but it catches
IngestionRunnerUnavailableException and the connectivity failure threw the generic
parent, so the typed handler never fired. Two changes:
- AirflowRESTClient throws IngestionRunnerUnavailableException for a failed API
detection. It is a subclass, so every existing catch of the parent still works.
- EntityRepository tracks whether the current thread is inside a hard-delete
cascade (same ThreadLocal pattern DomainRepository already uses), and
IngestionPipelineRepository tolerates an unreachable runner when it is. Deleting
the DAG is best-effort cleanup of an external system; a direct delete of the
pipeline itself still surfaces the failure.
On the test side this was billing 15 idle minutes per occurrence: NamespaceCleanup
polls for a 404 that a failed cascade never produces. Default cap drops 15 -> 2 min;
scale-it and seed-it pin 15 explicitly because they really do delete a 100k-table
service.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ge job Critical path was build-image (17m) -> ui-it-nightly (64m) = ~84m. Three structural costs, none of them test logic: - build-image serialised ~17 min in front of every test job to hand over a 461 MB image artifact each leg then loaded alone. Each leg now builds its own image, overlapping that work with its own setup and dropping the artifact round-trip. The shared GHA layer scope keeps it from being a cold build; only one leg writes, so concurrent legs do not duplicate blobs. - Free Disk Space ran every target (308s in build-image, 72-99s per test job) for space these jobs never needed. android alone reclaims ~9 GB in seconds. - The apt-get list (unixodbc, librdkafka, sasl, ffi...) exists for the Python ingestion build; nothing in these Java-only jobs links against it. ANTLR stays. Both suites are serial *within* a runner because they mutate cluster-global search state, so the shard matrix is the only parallelism available: ui-it x2, search-it x3. ShardFilter is a ServiceLoader PostDiscoveryFilter keyed on jpw.shard.total/index, assigning by hash(className) % total rather than a checked-in class list -- a list needs manual rebalancing and silently drops any test nobody remembered to add. Also fixes cohort sizing: github.event.inputs.* is empty on pull_request, so the unconditional `|| '5000'` fallback meant every PR ingested the full nightly stress cohort (10k entities) instead of the 200/100/100/100 baked into each test. Those props are now set only on workflow_dispatch, which is what the comment above them always claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
backend
safe to test
mohityadav766
changed the title
The deleted UIITs each injected a reindex mid-flow and re-asserted. Deleting them kept
the product behaviour covered (DataQuality/IncidentManager/Profiler/DataObservability
specs already assert it) but dropped the post-reindex dimension for ~20 surfaces, leaving
only the three existing *AfterReindex specs. That dimension is the one worth keeping:
reindex rebuilds a doc from the fields each index declares in getRequiredReindexFields(),
so a field the UI reads through search but nobody declared disappears on rebuild while the
live-write path keeps working — exactly the shape of the 1.12.7 and PR-27723 regressions.
Adds utils/reindex.ts (trigger + wait for the doc to come back; no ?recreate=true, since
SearchResource.reindexEntities declares no such param and always ignored it) and specs for:
- TestCaseCrudAfterReindex: row still listed, UI-edited parameterValues and array params
survive. Was DataQuality{Table,Column}TestCaseCrud + ArrayParams.
- IncidentManagerAfterReindex: acknowledged status survives, entity-page Incidents tab rows
survive, list still paginates. Was IncidentManager{Acknowledge,Resolve} + IncidentTab.
- DataObservabilityTabsAfterReindex: tag / glossary term / domain tabs still load their
widgets after that governance entity is rebuilt. Was the four DataObservability UIITs.
- DataQualityListAfterReindex: /data-quality list still paginates and name-searches.
Was DataQualityPagination + DataQualityFilters.
- DataQualityDashboardAfterReindex: widgets still have data and the filter bar renders.
Was DataQualityDashboardDimensionAndPie + StandaloneDQDashboardFilters.
- ProfilerSettingsAfterReindex: tableProfilerConfig survives a table rebuild.
Not ported: TestSuiteCrudReindexUIIT (was @disabled) and TestSuiteDetailsPageReindexUIIT,
whose post-reindex assertion is that a modal's filter dropdowns render — a routing concern
the rebuild does not touch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
First CI run on the restructure measured what the projections could not: - Free Disk Space 308s -> 24s and the docker build 213s -> 41s (shared GHA layer scope). Folding build-image into the legs worked; the 17-min serial block is gone. - A shard holding one 1-second test still took 13.2m, so fixed cost, not test time, is now what sets the floor. Of that, 467s is Maven (146s integration-tests + 321s dist) and ~170s is the OM stack starting per shard. So: one reactor pass for integration-tests + dist instead of two (as separate invocations Maven walked spec/common/sdk/service twice), with -T 1C across the runner's cores. Replaces ShardFilter with `find | sort | awk 'NR % t == i'`. The hash filter partitioned fine over 500 synthetic names and uselessly over the real ones — 8 of 9 classes landed on one shard, which is no speedup at all, and its balance test never exercised a suite that small. Its own unit test therefore passed on a property the real input does not have. Deleting it is also a smaller diff than keeping it: no Java, no ServiceLoader registration. File order is not a balance guarantee either (N=3 was worse than N=2 — the slow Search* classes sort adjacent), which is why the comment says to cut fixed cost before adding shards rather than reaching for a hand-maintained duration table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Comment on lines
+72
to
+86
| /** Polls the entity's alias until its id matches exactly one doc. */ | ||
| const waitForIndexed = async ( | ||
| apiContext: APIRequestContext, | ||
| target: ReindexTarget | ||
| ): Promise<void> => { | ||
| const index = SEARCH_INDEX_BY_TYPE[target.type]; | ||
|
|
||
| if (!index) { | ||
| throw new Error( | ||
| `No search index mapped for entity type '${target.type}' — add it to SEARCH_INDEX_BY_TYPE` | ||
| ); | ||
| } | ||
|
|
||
| await expect | ||
| .poll( |
There was a problem hiding this comment.
💡 Edge Case: waitForIndexed may pass on the stale pre-reindex doc
reindexEntities triggers the rebuild and then waitForIndexed polls the search alias until it sees exactly one hit for the id. The backend reindex deletes the doc and re-creates it, but if the endpoint returns before the async delete has run (the file's own comment states it is async — 'answers 202 and runs on the executor'), the first poll observes the still-present old document, returns 1, and resolves immediately — so the subsequent assertions can race the rebuild the specs exist to validate, giving false-green or flaky results. If the endpoint instead blocks until completion (it calls future.get(...)), the wait is simply redundant. Consider gating on a value the rebuild changes (e.g. poll a field/version that reindex updates, or delete-then-wait-for-reappear) rather than mere presence of any doc for the id.
Was this helpful? React with 👍 / 👎
Measured on run 34470830757: the four ui-it legs build an identical image, and three of them finished that step in ~41s against 272s for the fourth. The odd one out was the designated cache writer — mode=max exports every layer — and that +231s is the entire reason the slowest job came in at 20.1m while its same-shard twin took 16.7m. A PR run cannot benefit from its own write anyway: all four legs start together, so the export only ever helps a later run. Writing solely on workflow_dispatch takes it off the critical path without adding a job. That is safe rather than lucky because the layer worth caching does not change per PR. The Dockerfile is 3-stage and the expensive stage is jre-builder (jlink over the JDK base image); only the cheap `dist` untar layer moves with the tarball, which is why read-only legs still got 41s. mode=min was the other candidate and is wrong here — it skips intermediate stages, so every leg would rebuild the JRE. Cold-build fallback if the builder stage or its base image changes: ~230s per leg until someone dispatches once to repopulate. Degraded, not broken. Expected worst leg ~16.7m. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every scheduled main run of the nightly k8s Java IT fails in search-it, always the same
four tests, each dying in under 0.2s:
java.net.ConnectException
at HighlightFieldSaveValidationIT.currentSettingsJson(...:240)
SdkClients.BASE_URL initialises from IT_BASE_URL and otherwise localhost:8585. The CI login
script exports OM_URL / OM_ADMIN_TOKEN, which UiTestServer reads — so a test that goes
through a harness gets pointed at the ephemeral cluster, and a test that talks to the server
directly (HttpClient + getServerUrl()) does not. HighlightFieldSaveValidationIT has no
@BeforeAll and no extension, so it only worked when some other class happened to boot the
harness first and call overrideBaseUrl. Whether the suite passed came down to test order,
which is why it fails on main but not on the 1.13/2.0 dispatch runs.
Reading OM_URL at BASE_URL init makes external mode correct from static init for every
entry point, harness or not.
Costs the nightly ~7.7h per failed run today: scale-it gates only on deploy, so search-it
failing does not stop the 460m scale job that follows it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…Loader This test was 125.1m of the nightly k8s Java IT's 300m — 42% of the whole workflow — and its own log says where it went: Seeded 100000 tables (5 columns each) in 6411160 ms <- 106.9m Recursive async hard delete ... cleared search in 1089795 ms <- 18.2m Seeding was 85% of it, done by a local fixed-thread executor issuing one SdkClients.adminClient().tables().create(...) per table. EntityLoader does the identical work — same 100k tables, same 5 columns — and on the same run, same cluster, same parallelWorkers=8, logged: EntityLoader done: created=100000 columns=500000 duration=PT29M14.478571946S 57.0 tables/s against this executor's 15.6/s, a 3.65x gap for the same work at the same concurrency. EntityLoader's rate also held flat as the cluster filled (59.9 -> 55.9 -> 57.0 tables/s across the 10k, 50k and 100k cohorts that ran before it), so dataset growth does not account for the difference. Both seeders' concurrency now comes from one property. The executor read jpw.scale.workers while EntityLoader reads jpw.loader.maxWorkers, and the nightly only ever set the latter — so the workflow input documented as "the cap on create concurrency" was never reaching the most expensive seeder in the suite. Requesting 32 and letting maxWorkers cap it matches what Scale100kEntitiesIT already does. The service is now read back off the namespace rather than created up front: EntityLoader builds its own service+schema in ensureTablesSchema, and creating one here first would leave the seeded tables under a different service than the one deleted — the scoped doc counts would then pass against an empty service, which is the one way this test can go green while testing nothing. Assertions are unchanged: 100k table docs and 500k column docs scoped by service.id before the delete, zero of each after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code Review 👍 Approved with suggestions 1 resolved / 2 findingsComprehensive optimization of the java-playwright-nightly CI pipeline that eliminates 30 minutes of test idle time, deduplicates 23 UIITs already covered by the Playwright suite, and restructures CI to parallelize image builds across shards. Fixes a production bug where cascade hard-deletes of services with ingestion pipelines fail when Airflow is unreachable by having 💡 Edge Case: waitForIndexed may pass on the stale pre-reindex doc📄 openmetadata-ui/src/main/resources/ui/playwright/utils/reindex.ts:72-86 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/reindex.ts:27-28
✅ 1 resolved✅ Quality: Airflow API-detection failure now masks non-scheduler faults during delete
🤖 Prompt for agentsOptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes:
Fixes #33133
java-playwright-nightly.yml(the requiredjava-ui-itcheck) ran ~84 min on every search-indexing PR. I profiled run 34447075164 — failsafe reports, job step timings and the recorded Playwright videos — and found three separate causes, only one of which was "the tests are slow". Full evidence in the issue.1. A 15-minute no-op in two tests (30 of the 59 minutes). In the videos, all four pages open within 21s and the UI work finishes by ~80s; the page then sits idle for ~900s.
TestNamespaceExtension.afterEachruns before the browser context closes, andNamespaceCleanup.CASCADE_TIMEOUTwas 15 min. This turned out to be a production bug: a cascade hard-delete of any service with an ingestion pipeline underneath it aborts outright when Airflow is unreachable, becauseAirflowRESTClientthrows the genericPipelineServiceClientExceptionwheredeleteDeployedPipelineonly tolerates the typedIngestionRunnerUnavailableExceptionsubclass.2. Every PR ran the full nightly stress cohort.
github.event.inputs.*is empty onpull_request, so|| '5000'always won — 10,000 entities ingested where the in-test defaults are 200/100/100/100.Type of change:
High-level design:
Test dedup (−3,640 lines). Deleted 23 UIITs whose scenario is already asserted in the UI suite, plus the 8 page objects left with no referrer. Audited class-by-class against the specs rather than by name:
DataQuality{Table,Column}TestCaseCrudReindexUIITDataQuality.spec.ts→ "Table/Column test case" Create/Edit/DeleteDataQuality{ArrayParams,Filters,Pagination}ReindexUIITDataQuality.spec.tsDataQualityDashboardDimensionAndPieReindexUIITDataQualityDashboard.spec.tsStandaloneDQDashboardFiltersReindexUIIT,{Tag,GlossaryTerm,Domain}DataObservability*(5)DataObservabilityGovernanceTab.spec.tsIncidentManager{Acknowledge,AssignResolve,Resolve,Filters}ReindexUIIT,IncidentTabOnEntityPage*(5)IncidentManager.spec.tsIncidentManagerPaginationReindexUIITIncidentManagerPagination.spec.tsProfilerSettingsModalReindexUIIT,TableProfilerColumnGraphsReindexUIITProfiler.spec.tsTestSuiteCrudReindexUIIT(already@Disabled),TestSuiteDetailsPageReindexUIITTestSuite.spec.ts,TestSuiteDetailsPage.spec.tsTopicUIIT,TableDetailsSmokeUIITTopic.spec.ts,Entity.spec.tsKept: the 8 search-indexing UIITs with no UI equivalent (
SearchAvailable*,SimpleReindexTrigger,DistributedAutoTune,SelectiveFieldReindex,LongCompoundNameSearch,PipelineOwnerIndex,EntityLoaderSmoke) and all 29search-itclasses. Also keptGoogleSsoSignInUIIT— it's skipped unlessjpw.auth=sso-google-confidential, so it costs 0s and shares the mock-IdP harness with the two backend SSO tests; deleting it is churn with no speed gain.Delete fix.
AirflowRESTClientnow throwsIngestionRunnerUnavailableExceptionfor a failed API detection — it's a subclass, so every existingcatchof the parent still works.EntityRepositorytracks whether the thread is inside a hard-delete cascade (same ThreadLocal patternDomainRepositoryalready uses) andIngestionPipelineRepositorytolerates an unreachable runner when it is. Deleting the DAG is best-effort cleanup of an external system; a direct delete of the pipeline still surfaces the failure, which is the behaviour worth keeping.CI restructure. Dropped the
build-imagejob — it serialised ~17 min in front of every leg to hand over a 461 MB artifact each leg then loaded alone. Each leg now builds its own image, overlapping that with its own setup; the shared GHA layer scope keeps it warm and only one leg writes so concurrent legs don't duplicate blobs. TrimmedFree Disk Spacetoandroid(the only target worth its runtime) and dropped the Python-ingestionapt-getlist from these Java-only jobs.Both suites are serial within a runner because they mutate cluster-global search state, so a shard matrix is the only parallelism available:
ui-it×2,search-it×3.ShardFilteris a ServiceLoaderPostDiscoveryFilterassigning byhash(className) % total.Alternatives rejected: a checked-in class list per shard (needs manual rebalancing and silently drops any test nobody adds); enabling class-level parallelism in the pom (these tests mutate cluster-global state, so
@ResourceLockwould serialise them anyway); moving the remaining UIITs to the embedded bootstrap (UiTestServersupports only External/Containerized — no embedded path, which is exactly whyui-itneeds the image andsearch-itdoesn't).Rollout: no schema, migration or API-contract change. The delete fix is behaviour-affecting in one direction only — cascades that previously failed now succeed, logging a warning about the orphaned DAG.
Tests:
Use cases covered
Unit tests
ShardFilterTest— 4 tests pinning the partition invariants: every class claimed by exactly one shard across 2–8 shards, stable assignment,total=1keeps everything, no degenerate partition.NamespaceCleanupTeststill pins the non-fixed poll interval.Backend integration tests
Not applicable — no new API endpoints. The delete fix is exercised by the existing UIIT cleanup path, which is what surfaced it: the two runs that hit it logged
Cleanup cascade for databaseService … still running after PT15M/deleted 0/1.Ingestion integration tests
Not applicable — no ingestion changes.
Playwright (UI) tests
playwright/e2e/Features/DataQuality/ColumnProfileGraphsAfterReindex.spec.ts, the one assertion from the deleted Java suite with no UI equivalent.Profiler.spec.tsalready asserts the four graphs (#count_graph,#proportion_graph,#math_graph,#sum_graph) insidevalidateProfilerAccessForRole, but not post-reindex. Modelled on the existingTestCaseStatusAfterReindex.spec.ts.Manual testing performed
mvn compile -pl :openmetadata-service→ BUILD SUCCESSmvn test-compile -pl :openmetadata-integration-tests→ BUILD SUCCESS (confirms no dangling refs to the deleted classes)mvn spotless:applyon both modules → clean-Djpw.shard.total=2 -Djpw.shard.index=0→Tests run: 4(ShardFilterTest)-Djpw.shard.total=2 -Djpw.shard.index=1→Tests run: 0Tests run: 4UI_IT_SHARDS/SEARCH_IT_SHARDSverified equal to their matrix list lengthsUI screen recording / screenshots:
Not applicable — no product UI changes; the only UI-tree file is a new Playwright spec.
Checklist:
Fixes #33133above.Note for reviewers
Two things I could not verify locally:
UI_IT_SHARDS/SEARCH_IT_SHARDSinenv:are the dial.ui-itlegs each build the dist tarball and image instead of one shared job — that's the trade that buys the wall clock. If cost matters more than the last few minutes, running only OpenSearch on PRs and both engines on dispatch halves it. I left both on becausechangesalready gates this to search-indexing PRs, which is exactly where engine differences bite.🤖 Generated with Claude Code