Fixes #27418: optimize test case hard-delete relationship cleanup#27633
Fixes #27418: optimize test case hard-delete relationship cleanup#27633Megh-Shah-08 wants to merge 3 commits intoopen-metadata:mainfrom
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! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Optimizes cleanup of testCaseResolutionStatus relationships when deleting TestCase entities (and during retention cleanup) to avoid per-relationship deletion overhead and prevent orphaned relationship rows.
Changes:
- Added a batch relationship cleanup path for all resolution-status records associated with a TestCase FQN.
- Hooked the optimized cleanup into TestCase hard-delete lifecycle and the Data Retention app.
- Updated time-series hard-delete behavior to also remove relationships; added an integration test covering the cleanup behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseResolutionStatusRepository.java | Adds bulk relationship cleanup by TestCase and changes relationship type used for linking TestCase ↔ ResolutionStatus. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java | Calls the new resolution-status relationship cleanup during TestCase entity-specific hard-delete cleanup. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityTimeSeriesRepository.java | Ensures hard-deleting a time-series record also deletes its relationships. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/CollectionDAO.java | Adds DAO method to delete orphaned relationships based on missing “from” entity rows. |
| openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/dataRetention/DataRetention.java | Adds retention-job step to remove orphaned TestCase → ResolutionStatus relationship rows. |
| openmetadata-service/src/main/java/org/openmetadata/service/Entity.java | Special-cases time-series entities in Entity.deleteEntity to avoid standard delete flow. |
| openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java | Adds integration test asserting relationships are removed while time-series history remains. |
| @Transaction | ||
| private void cleanOrphanedTestCaseResolutionStatusRelationships() { | ||
| LOG.info("Initiating cleanup for orphaned testCaseResolutionStatus relationships."); | ||
| executeWithStatsTracking( | ||
| "orphaned_test_case_resolution_status_relationships", | ||
| () -> | ||
| collectionDAO | ||
| .relationshipDAO() | ||
| .deleteOrphanedRelationships( | ||
| Entity.TEST_CASE, Entity.TEST_CASE_RESOLUTION_STATUS, "test_case")); |
There was a problem hiding this comment.
cleanOrphanedTestCaseResolutionStatusRelationships is wired into executeWithStatsTracking, which is designed for batched deletes (BATCH_SIZE per call). However relationshipDAO().deleteOrphanedRelationships(...) performs an unbounded delete in a single statement, which can lead to long transactions/table locks on large entity_relationship tables. Consider implementing a batched variant (delete with LIMIT / ctid selection) or integrating this cleanup into the existing EntityRelationshipCleanupUtil batching approach so the retention job remains predictable.
There was a problem hiding this comment.
Good point. This currently uses a single set-based delete, which is efficient and consistent with existing cleanup patterns. We can extend it to a batched approach if needed for larger datasets in a follow-up.
…e relationship cleanup
|
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! |
Code Review 🚫 Blocked 1 resolved / 2 findingsOptimizes relationship cleanup for test case hard-deletes, but the changes to 🚨 Bug: PARENT_OF→RELATED_TO change breaks existing data without migration📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseResolutionStatusRepository.java:260 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseResolutionStatusRepository.java:268 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseResolutionStatusRepository.java:239-247 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseResolutionStatusRepository.java:256 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseResolutionStatusRepository.java:264 The relationship type between TestCase and TestCaseResolutionStatus is changed from This means:
A SQL migration script is needed to update existing rows: UPDATE entity_relationship
SET relation = 15
WHERE fromEntity = 'testCase'
AND toEntity = 'testCaseResolutionStatus'
AND relation = 9;Suggested fix✅ 1 resolved✅ Quality: Unused variable in Entity.deleteEntity early return
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Hi @ayush-shah, I’ve addressed the majority of the Copilot and Gitar review comments, including performance improvements and backward compatibility concerns. Thanks! |
ayush-shah
added
the
safe to test
🟡 Playwright Results — all passed (11 flaky)✅ 3700 passed · ❌ 0 failed · 🟡 11 flaky · ⏭️ 89 skipped
🟡 11 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
| TestCaseResolutionStatusRepository testCaseResolutionStatusRepository = | ||
| (TestCaseResolutionStatusRepository) | ||
| Entity.getEntityTimeSeriesRepository(Entity.TEST_CASE_RESOLUTION_STATUS); | ||
| testCaseResolutionStatusRepository.deleteAllRelationshipsByTestCase(entityInterface.getId()); |
There was a problem hiding this comment.
deleteAllRelationshipsByTestCase(...) is likely redundant in the hard-delete path: EntityRepository.cleanup() already calls relationshipDAO().deleteAll(testCaseId, Entity.TEST_CASE), which deletes all fromEntity=testCase rows (including testCase -> testCaseResolutionStatus). If the intent is performance, this extra delete doesn’t avoid the expensive part (the pre-cleanup deleteChildren(...) traversal still runs). Consider removing this call, and if you want to avoid the recursive child traversal/logging for large incident histories, override deleteChildren(...) in TestCaseRepository to skip time-series child types and rely on cleanup() + targeted relationship cleanup instead.
| TestCaseResolutionStatusRepository testCaseResolutionStatusRepository = | |
| (TestCaseResolutionStatusRepository) | |
| Entity.getEntityTimeSeriesRepository(Entity.TEST_CASE_RESOLUTION_STATUS); | |
| testCaseResolutionStatusRepository.deleteAllRelationshipsByTestCase(entityInterface.getId()); |
| if (entityType.equalsIgnoreCase(Entity.TEST_CASE_RESOLUTION_STATUS) | ||
| || entityType.equalsIgnoreCase(Entity.TEST_CASE_RESULT)) { | ||
| // TimeSeries entities are cleaned up via entitySpecificCleanup, | ||
| // not through the standard repository delete flow. |
There was a problem hiding this comment.
Entity.deleteEntity(...) now silently no-ops for testCaseResolutionStatus / testCaseResult. Since this is a public utility used by recursive deletion, a silent return can mask unexpected calls and make debugging difficult. Consider either (a) routing to the relevant EntityTimeSeriesRepository.deleteById(...) when hardDelete=true, or (b) logging at least a debug/warn when skipping, so callers know the entity was intentionally not deleted via the standard repository flow.
| // not through the standard repository delete flow. | |
| // not through the standard repository delete flow. | |
| LOG.warn( | |
| "Skipping standard delete flow for time series entity type {} with id {} (hardDelete={}); " | |
| + "cleanup is expected to happen via entitySpecificCleanup.", | |
| entityType, | |
| entityId, | |
| hardDelete); |
| assertTrue( | ||
| statusCount >= 3, | ||
| "There should be at least 3 relationships to resolution statuses before delete"); |
There was a problem hiding this comment.
The pre-delete assertion is very loose: statusCount >= 3 will still pass if duplicate/extra testCase -> testCaseResolutionStatus relationships are accidentally created. Since this test creates exactly 3 statuses, assert the exact expected count so the test reliably catches regressions.
| assertTrue( | |
| statusCount >= 3, | |
| "There should be at least 3 relationships to resolution statuses before delete"); | |
| assertEquals( | |
| 3, | |
| statusCount, | |
| "There should be exactly 3 relationships to resolution statuses before delete"); |
|
Describe your changes:
Fixes #27418
Summary: Optimized the cleanup of TestCaseResolutionStatus relationships during TestCase hard-deletes. Replaced an inefficient$O(N)$ sequential deletion loop with a single set-based SQL delete.
Root Cause: The repository was iterating through each relationship and deleting them one-by-one. This caused significant database round-trip overhead for test cases with large incident histories.
Changes:
How I tested:
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
Relationship.PARENT_OFforTestCasetoTestCaseResolutionStatusassociations.CollectionDAO.deleteOrphanedRelationshipsquery performance by switching toNOT EXISTSinstead ofNOT IN.recursive=trueparameter toTestCasedeletion inTestCaseResourceITto ensure proper cascading cleanup.This will update automatically on new commits.