Skip to content

Fixes 30008: Improve server startup and write-path latency - #30009

Open
harshach wants to merge 18 commits into
mainfrom
harshach/perf-analysis-startup-latency
Open

Fixes 30008: Improve server startup and write-path latency#30009
harshach wants to merge 18 commits into
mainfrom
harshach/perf-analysis-startup-latency

Conversation

@harshach

@harshach harshach commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Describe your changes:

Fixes #30008

I improved server startup, request/write latency, and lineage query performance because warm boots repeated bundled-data and search reconciliation, writes repeated JSON serialization, and the pipeline-summary service type was interpolated into SQL.

Type of change:

  • Improvement

High-level design:

  • Startup observability: ordered step timings, slow-resource attribution, standard request-latency buckets, and opt-in percentile histograms
  • Warm-start path: SHA-256 gates for bundled seed data and search templates, an indexed hard-row presence check for every required bundled seed entity, failure-safe retries, bounded parallel index initialization, and database-backed type-registry hydration
  • Classpath/auth path: one shared ClassGraph scan, one cached classpath resource index, and bounded RBAC SpEL memoization
  • Write path: direct Jakarta JSON-P conversion, replayable TokenBuffer retry snapshots, storage-JSON reuse for cache write-through, and lifecycle snapshot gating
  • Database path: bound pipeline-summary filters plus MySQL generated-column and PostgreSQL expression indexes for pipeline lineage
  • Rollout: startup gates default on with force/disable controls; fingerprint persistence uses the existing settings table; lineage DDL is supplied for both databases
  • Compatibility: AI control FQNs are now framework-qualified; this entity is only on unreleased 2.0.0-SNAPSHOT, so no production migration is required, but development databases that already seeded flat control FQNs should be reset
  • Alternatives: duplicate MySQL pipeline-name DDL was rejected because migration 1.2.0 already creates it; Flowable/Quartz deferral remains evidence-gated behind the new timing data

Tests:

Use cases covered

  • First, unchanged, changed, failed, and forced seed/template fingerprint decisions
  • Matching seed fingerprints with hard-missing required rows rerun seed loaders; matching search checksums verify live template markers; probe failures fail open and intentionally deletable seeds remain optional
  • Parallel index creation and template acknowledgement/failure handling
  • PATCH conversion equivalence, replayable retry state, cache JSON reuse, and lifecycle snapshot suppression
  • Pipeline-summary filtering/pagination and quote-containing service-type input
  • Pipeline-reference lineage relation precedence and source-lineage deletion
  • Forced-deadlock update replay without double version bumps or consolidation corruption

Unit tests

  • I added unit tests for the new/changed logic.
  • Files added/updated: CommonUtilTest, StartupTimerTest, SeedDataGateTest, FrameworkSeedLoaderTest, ClasspathScanIndexTest, CollectionRegistryTest, RBACConditionEvaluatorCacheTest, IndexTemplateManagerTest, EntityRepositoryStorageJsonReuseTest, EntityLifecycleEventDispatcherTest, RequestLatencyContextTest, MicrometerBundleTest, SettingsRowMapperTest, and JsonUtilsTest
  • Final review-focused result: SeedDataGateTest and FrameworkSeedLoaderTest passed 9/9 combined; IndexTemplateManagerTest passed 10/10
  • Coverage %: not measured

Backend integration tests

  • I added integration tests in openmetadata-integration-tests/ for new/changed backend behavior.
  • Files added/updated: PipelineResourceIT.java, LineageResourceIT.java, SeedDataPresenceIT.java, AIFrameworkControlResourceIT.java, and IndexTemplateIT.java
  • MySQL 8.3 + Elasticsearch 9.3: 3 pipeline/lineage tests passed
  • PostgreSQL 15 + OpenSearch 3.4: 3 pipeline/lineage tests passed
  • MySQL 8.3 + Elasticsearch 9.3: 2 forced-deadlock replay tests passed
  • The seed-presence, AI control collision, and search-template integration sources compile; local container execution was blocked before test discovery because exhausted Docker VM storage prevented Testcontainers from creating a container

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Not applicable (no UI changes).

Manual testing performed

  1. Ran clean specification generation/install and compiled service plus integration-test sources
  2. Ran MySQL and PostgreSQL migration smoke checks
  3. Ran mvn spotless:apply, mvn spotless:check, schema validation, and git diff --check

UI screen recording / screenshots:

Not applicable.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • For UI changes: not applicable; this PR has no UI changes.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.
  • I have added tests around the new logic.

Greptile Summary

This PR improves startup gating, search setup, write latency, and lineage query performance. The main changes are:

  • Seed-data startup skips now use checksums plus database row checks.
  • Search-template startup skips now compare live template fingerprints.
  • Entity write paths reuse JSON snapshots and retry buffers.
  • Pipeline summary filters are bound and lineage queries get new indexes.
  • Startup timing and request-latency metrics were expanded.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The seed-data gate now checks required database rows before skipping reconciliation.
  • The search-template gate now rebuilds when live templates are missing, stale, or unreadable.

Important Files Changed

Filename Overview
openmetadata-service/src/main/java/org/openmetadata/service/seeding/SeedDataGate.java Adds startup fingerprint checks for seed data and search templates, with database row drift detection before skipping seed reconciliation.
openmetadata-service/src/main/java/org/openmetadata/service/seeding/RequiredSeedRows.java Adds the seed-row manifest used to compare bundled seed resources with rows present in the database.
openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java Adds live search-template fingerprint checks before warm-start template updates are skipped.
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java Adds the seed-row count query and updates pipeline summary filtering to use bound service-type values.

Reviews (17): Last reviewed commit: "Merge branch 'main' into harshach/perf-a..." | Re-trigger Greptile

Context used (3)

  • Context used - openmetadata-ui-core-components/CLAUDE.md (source)
  • Context used - CLAUDE.md (source)
  • Context used - AGENTS.md (source)

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Jul 14, 2026
Comment thread common/src/main/java/org/openmetadata/common/utils/CommonUtil.java
@harshach
harshach marked this pull request as ready for review July 14, 2026 04:22
Copilot AI review requested due to automatic review settings July 14, 2026 04:22
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown
Contributor

✅ TypeScript Types Auto-Updated

The generated TypeScript types have been automatically updated based on JSON schema changes in this PR.

Copilot AI review requested due to automatic review settings July 14, 2026 04:26
@github-actions
github-actions Bot requested a review from a team as a code owner July 14, 2026 04:26
@harshach
harshach removed the request for review from Copilot July 14, 2026 04:26
Copilot AI review requested due to automatic review settings July 14, 2026 04:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.67% (76798/116936) 49.54% (46114/93066) 50.75% (13890/27365)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gitar-bot

gitar-bot Bot commented Jul 19, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 7 resolved / 7 findings

Improves server startup and write-path performance through fingerprint-gated seeding, parallelized index initialization, and shared classpath scanning. Resolved all identified seed drift and framework qualification issues during the review session.

✅ 7 resolved
Bug: CommonUtil.getResources now returns an immutable list

📄 common/src/main/java/org/openmetadata/common/utils/CommonUtil.java:63-77
getResources() now returns stream()...toList() over an immutable index (buildClasspathResourceIndex uses List.copyOf), whereas it previously returned a mutable ArrayList. Any caller that mutates the returned list (add/remove/sort) will now throw UnsupportedOperationException at runtime. The new CommonUtilTest even asserts this immutability. Please confirm no downstream caller of getResources()/getJsonDataResources() mutates the result; if any does, wrap it in a new ArrayList or fix the caller.

Performance: Search-template gate never stamps fingerprint if any mapping is missing

📄 openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java:582 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java:595-599 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java:607-621
In createOrUpdateIndexTemplates, failed is initialized to entityIndexMap.size() - templates.size(). buildIndexTemplateDefinitions() silently drops any entity whose mapping content is null or fails to read, so templates.size() can be permanently smaller than entityIndexMap.size(). That makes failed > 0 on every boot, which triggers recordSearchTemplateFailure() (resetting the stored fingerprint to null). As a result the search-template gate never records a clean fingerprint and templates are rebuilt on every startup, defeating the warm-start optimization this PR adds. Consider only counting genuine per-template failures (increment inside the catch block) rather than pre-charging failed with the count of skipped/no-mapping entities.

Quality: ClasspathScanIndex keeps ScanResult open for JVM lifetime

📄 openmetadata-service/src/main/java/org/openmetadata/service/util/ClasspathScanIndex.java:32-39 📄 openmetadata-service/src/main/java/org/openmetadata/service/util/ClasspathScanIndex.java:45-57
The new ClasspathScanIndex singleton opens a ClassGraph ScanResult in its constructor and never closes it, whereas the previous CollectionRegistry/Entity code wrapped the scan in try-with-resources. The loaded classes are cached in the two maps, so the ScanResult itself no longer needs to stay open after warmup; retaining it holds ClassGraph's class-graph metadata (and any file handles) for the life of the process. Consider closing the ScanResult after populating the caches, or eagerly materializing the needed lookups and closing the scan.

Edge Case: Seed-data gate skips reseeding on DB drift when files are unchanged

📄 openmetadata-service/src/main/java/org/openmetadata/service/seeding/SeedDataGate.java:96-101 📄 openmetadata-service/src/main/java/org/openmetadata/service/seeding/SeedDataGate.java:173-187 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:1408-1413
SeedDataGate.shouldSeed() decides whether to seed based solely on a fingerprint of the classpath seed resources plus server version, compared to the fingerprint persisted in the settings table. If seed rows (roles, policies, bootstrap types, etc.) are removed or corrupted out-of-band while the seed files remain unchanged, a restart will skip reseeding and leave the system missing required seed data. This is mitigated by STARTUP_FORCE_SEED_DATA / seedDataGateEnabled=false, but the failure mode is silent. Consider documenting this operational caveat prominently, or gating on a lightweight presence check in addition to the file fingerprint.

Edge Case: Warm-start seed gate reseeds every boot when email config is unset

📄 openmetadata-service/src/main/java/org/openmetadata/service/seeding/SeedDataGate.java:313-324 📄 openmetadata-service/src/main/java/org/openmetadata/service/seeding/RequiredSeedRows.java:55-67
When no EMAIL_CONFIGURATION settings row exists, SeedDataGate.getEmailTemplateProvider() returns null (SeedDataGate.java:316-317), and RequiredSeedRows.selectEmailDocuments(null) then requires BOTH openmetadata and collate email documents to be present (RequiredSeedRows.java:58-60). However the actual seeder DocumentRepository.getEntitiesFromSeedData() loads only ONE provider's templates — defaulting to openmetadata (DocumentRepository.java:64-75) via SettingsCache which returns the OPENMETADATA default rather than null. As a result the collate email documents are never seeded, hasRequiredSeedRows always returns false, and the drift check reseeds on every warm boot — defeating the startup optimization this PR introduces. Make getEmailTemplateProvider default to OPENMETADATA when the config row is absent (mirroring the seeder / DefaultOperationalConfigProvider), instead of returning null.

...and 2 more resolved from earlier reviews

Options

Display: compact → Showing less information.

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

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

…s-startup-latency

# Conflicts:
#	bootstrap/sql/migrations/native/2.0.0/mysql/schemaChanges.sql
#	bootstrap/sql/migrations/native/2.0.0/postgres/schemaChanges.sql
#	openmetadata-service/src/main/java/org/openmetadata/service/events/lifecycle/EntityLifecycleEventDispatcher.java
#	openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java
#	openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java
#	openmetadata-spec/src/main/resources/json/schema/settings/settings.json
#	openmetadata-ui/src/main/resources/ui/src/generated/settings/settings.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown
Contributor

✅ TypeScript Types Auto-Updated

The generated TypeScript types have been automatically updated based on JSON schema changes in this PR.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…s-startup-latency

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 85 out of 87 changed files in this pull request and generated no new comments.

@sonarqubecloud

Copy link
Copy Markdown

@gitar-bot

gitar-bot Bot commented Jul 27, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 10 resolved / 12 findings

Improves server startup performance and write-path latency through fingerprint-gated seed reconciliation, shared classpath scanning, and direct JSON conversion. Consider refining the JSON exception mapping and record-batch timestamp tie handling to prevent potential 412 error misclassifications and duplicate rows.

💡 Quality: JsonException mapped to misleading concurrent-change 412 message

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SystemRepository.java:427-431

A JsonException from JsonUtils.applyPatch is typically raised for an inapplicable/malformed patch (e.g. a failing test op or a replace on a missing path), not a concurrent modification. Mapping it to PreconditionFailedException with the message "Glossary term relation settings changed while the JSON Patch was being applied" returns HTTP 412 with a cause that misleads clients who actually sent a bad patch. Consider distinguishing patch-application failures (400 Bad Request with a patch-specific message) from the genuine optimistic-concurrency case handled at the updatedRows==0 check.

💡 Edge Case: getLatestRecordBatch can return duplicate rows on timestamp ties

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java:10293-10305

The new getLatestRecordBatchInternal joins on t1.timestamp = t2.maxTs, so if two resolution-status rows for the same entityFQNHash share the maximum timestamp, both rows are returned. Consumers in TestCaseRepository insert into a HashMap keyed by FQN hash, so one row silently overwrites the other, making the selected 'latest' incident/result non-deterministic. Consider tie-breaking on the row id (e.g. also select MAX(id) for the max timestamp) if identical timestamps are possible; otherwise this is benign.

✅ 10 resolved
Bug: CommonUtil.getResources now returns an immutable list

📄 common/src/main/java/org/openmetadata/common/utils/CommonUtil.java:63-77
getResources() now returns stream()...toList() over an immutable index (buildClasspathResourceIndex uses List.copyOf), whereas it previously returned a mutable ArrayList. Any caller that mutates the returned list (add/remove/sort) will now throw UnsupportedOperationException at runtime. The new CommonUtilTest even asserts this immutability. Please confirm no downstream caller of getResources()/getJsonDataResources() mutates the result; if any does, wrap it in a new ArrayList or fix the caller.

Performance: Search-template gate never stamps fingerprint if any mapping is missing

📄 openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java:582 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java:595-599 📄 openmetadata-service/src/main/java/org/openmetadata/service/search/SearchRepository.java:607-621
In createOrUpdateIndexTemplates, failed is initialized to entityIndexMap.size() - templates.size(). buildIndexTemplateDefinitions() silently drops any entity whose mapping content is null or fails to read, so templates.size() can be permanently smaller than entityIndexMap.size(). That makes failed > 0 on every boot, which triggers recordSearchTemplateFailure() (resetting the stored fingerprint to null). As a result the search-template gate never records a clean fingerprint and templates are rebuilt on every startup, defeating the warm-start optimization this PR adds. Consider only counting genuine per-template failures (increment inside the catch block) rather than pre-charging failed with the count of skipped/no-mapping entities.

Quality: ClasspathScanIndex keeps ScanResult open for JVM lifetime

📄 openmetadata-service/src/main/java/org/openmetadata/service/util/ClasspathScanIndex.java:32-39 📄 openmetadata-service/src/main/java/org/openmetadata/service/util/ClasspathScanIndex.java:45-57
The new ClasspathScanIndex singleton opens a ClassGraph ScanResult in its constructor and never closes it, whereas the previous CollectionRegistry/Entity code wrapped the scan in try-with-resources. The loaded classes are cached in the two maps, so the ScanResult itself no longer needs to stay open after warmup; retaining it holds ClassGraph's class-graph metadata (and any file handles) for the life of the process. Consider closing the ScanResult after populating the caches, or eagerly materializing the needed lookups and closing the scan.

Edge Case: Seed-data gate skips reseeding on DB drift when files are unchanged

📄 openmetadata-service/src/main/java/org/openmetadata/service/seeding/SeedDataGate.java:96-101 📄 openmetadata-service/src/main/java/org/openmetadata/service/seeding/SeedDataGate.java:173-187 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:1408-1413
SeedDataGate.shouldSeed() decides whether to seed based solely on a fingerprint of the classpath seed resources plus server version, compared to the fingerprint persisted in the settings table. If seed rows (roles, policies, bootstrap types, etc.) are removed or corrupted out-of-band while the seed files remain unchanged, a restart will skip reseeding and leave the system missing required seed data. This is mitigated by STARTUP_FORCE_SEED_DATA / seedDataGateEnabled=false, but the failure mode is silent. Consider documenting this operational caveat prominently, or gating on a lightweight presence check in addition to the file fingerprint.

Edge Case: Warm-start seed gate reseeds every boot when email config is unset

📄 openmetadata-service/src/main/java/org/openmetadata/service/seeding/SeedDataGate.java:313-324 📄 openmetadata-service/src/main/java/org/openmetadata/service/seeding/RequiredSeedRows.java:55-67
When no EMAIL_CONFIGURATION settings row exists, SeedDataGate.getEmailTemplateProvider() returns null (SeedDataGate.java:316-317), and RequiredSeedRows.selectEmailDocuments(null) then requires BOTH openmetadata and collate email documents to be present (RequiredSeedRows.java:58-60). However the actual seeder DocumentRepository.getEntitiesFromSeedData() loads only ONE provider's templates — defaulting to openmetadata (DocumentRepository.java:64-75) via SettingsCache which returns the OPENMETADATA default rather than null. As a result the collate email documents are never seeded, hasRequiredSeedRows always returns false, and the drift check reseeds on every warm boot — defeating the startup optimization this PR introduces. Make getEmailTemplateProvider default to OPENMETADATA when the config row is absent (mirroring the seeder / DefaultOperationalConfigProvider), instead of returning null.

...and 5 more resolved from earlier reviews

🤖 Prompt for agents
Code Review: Improves server startup performance and write-path latency through fingerprint-gated seed reconciliation, shared classpath scanning, and direct JSON conversion. Consider refining the JSON exception mapping and record-batch timestamp tie handling to prevent potential 412 error misclassifications and duplicate rows.

1. 💡 Quality: JsonException mapped to misleading concurrent-change 412 message
   Files: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/SystemRepository.java:427-431

   A JsonException from JsonUtils.applyPatch is typically raised for an inapplicable/malformed patch (e.g. a failing test op or a replace on a missing path), not a concurrent modification. Mapping it to PreconditionFailedException with the message "Glossary term relation settings changed while the JSON Patch was being applied" returns HTTP 412 with a cause that misleads clients who actually sent a bad patch. Consider distinguishing patch-application failures (400 Bad Request with a patch-specific message) from the genuine optimistic-concurrency case handled at the updatedRows==0 check.

2. 💡 Edge Case: getLatestRecordBatch can return duplicate rows on timestamp ties
   Files: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java:10293-10305

   The new `getLatestRecordBatchInternal` joins on `t1.timestamp = t2.maxTs`, so if two resolution-status rows for the same `entityFQNHash` share the maximum timestamp, both rows are returned. Consumers in TestCaseRepository insert into a HashMap keyed by FQN hash, so one row silently overwrites the other, making the selected 'latest' incident/result non-deterministic. Consider tie-breaking on the row id (e.g. also select MAX(id) for the max timestamp) if identical timestamps are possible; otherwise this is benign.

Options

Display: compact → Showing less information.

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

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 85 out of 87 changed files in this pull request and generated 1 comment.

Comment on lines +745 to +748
} catch (Exception exception) {
failed++;
LOG.warn("Failed to create index template for {}", entry.getKey(), e);
LOG.warn(
"Failed to create index template for {}: {}", entry.getKey(), exception.getMessage());
@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 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.

Improve server startup and write-path latency

2 participants