Skip to content

Fix charts getting wrong service during reindex#26011

Merged
pmbrull merged 5 commits intomainfrom
fix/chart-service-bulk-load-
Feb 20, 2026
Merged

Fix charts getting wrong service during reindex#26011
pmbrull merged 5 commits intomainfrom
fix/chart-service-bulk-load-

Conversation

@sonika-shah
Copy link
Copy Markdown
Collaborator

@sonika-shah sonika-shah commented Feb 20, 2026

Describe your changes:

Fixes

Bug Summary

Problem: During reindexing, charts were getting incorrect service data in the search index. A chart belonging to TestMetabase would incorrectly show test_pbi as its service.

Root Cause: In ChartRepository.java, the fetchAndSetServices() method was registered as a field fetcher and incorrectly assumed all charts in a batch have the same service:

// BUGGY CODE (removed)

  private void fetchAndSetServices(List<Chart> charts, Fields fields) {                                                                                                        
    EntityReference service = getContainer(charts.get(0).getId());                                                                                                             
    for (Chart chart : charts) {                                                                                                                                               
      chart.setService(service);  // ALL charts got first chart's service!                                                                                                     
    }                                                                                                                                                                          
  }   

Fix: Removed the redundant fetchAndSetServices field fetcher registration because:

  • Bulk operations: fetchAndSetDefaultService() in setFieldsInBulk already handles it correctly
  • Single entity: setFields() already handles it via getContainer()

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • 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.

Summary by Gitar

  • Fixed bug: Removed fetchAndSetServices() field fetcher that incorrectly assigned all charts in a batch to the first chart's service
  • Test coverage: Added regression test test_bulkListChartsFromDifferentServices_maintainsCorrectServiceReference() validating charts retain correct service during bulk operations
  • UI test fix: Updated ExploreTree.spec.ts to use dynamic service names instead of hardcoded values, fixing flaky test dependency

This will update automatically on new commits.

@aniketkatkar97 aniketkatkar97 added UI UI specific issues To release Will cherry-pick this PR into the release branch labels Feb 20, 2026
@sonika-shah
Copy link
Copy Markdown
Collaborator Author

@gitar-bot addressed feedback , review changes again

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Feb 20, 2026

@gitar-bot addressed feedback , review changes again

I've reviewed the changes after your feedback was addressed. The code review shows no unresolved findings - the feedback has been successfully incorporated in commit 7fa82f5.

Current Status:

  • ✅ Code review: No issues found
  • ✅ Most CI checks: Passing (CodeQL, checkstyle, maven, python tests, MCP tests, etc.)
  • ⚠️ One flaky test failure in integration-tests-postgres-opensearch: LineageResourceIT.java:838 - This is unrelated to your Chart service fix. It's an OpenSearch version conflict (race condition) that occurs intermittently in the test infrastructure.

The changes look good to merge. The single test failure is a known flaky test issue with OpenSearch version conflicts during concurrent index updates, not caused by your code changes.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 20, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.77% (56483/85884) 45.21% (29573/65419) 48.03% (8924/18581)

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Feb 20, 2026

🔍 CI failure analysis for 212e3cc: TopicResourceIT search index timeout (new test type failing). ExploreTree test remains fixed. All failures continue to be flaky search index/infrastructure issues unrelated to PR.

Issue

Latest run (commit 212e3cc, job 64301856260):

  • integration-tests-mysql-elasticsearch: TopicResourceIT.java:1 failed

Previous runs (commit 212e3cc):

  • EntityVersionPages.spec.ts:82 (Playwright visibility)
  • DomainResourceIT.java:889, 1081 (search index timeouts)
  • DomainResourceIT, GlossaryTermResourceIT, ChartResourceIT (search index timeouts)

Context:

  • Commit 212e3cc fixed ExploreTree.spec.ts test

Root Cause

1. TopicResourceIT.java:1 (new entity type, flaky, unrelated):

Condition with alias 'Wait for search to reflect update' didn't complete within 1 minutes
Updated description should be in search index ==> expected: <true> but was: <false>

Why flaky/unrelated:

  • Topic entity description update not reflected in search index within 1 minute
  • Same error pattern as DomainResourceIT, GlossaryTermResourceIT, ChartResourceIT
  • Asynchronous Elasticsearch indexing timing issue
  • PR modifies Chart repository bulk operations, not Topic entities
  • This is another entity type exhibiting the same infrastructure search indexing issue

2. Backend integration test failures (flaky, unrelated):

DomainResourceIT:

  • Failed at 7 different lines: 1, 886, 889, 1078, 1081, 1087
  • Highly flaky search index timeouts

ChartResourceIT.java:1:

  • Search index timeout (infrastructure issue, not PR logic issue)
  • Same pattern as other search index failures

GlossaryTermResourceIT, GlossaryResourceIT:

  • Search index timeouts (1 minute)

TopicResourceIT (NEW):

  • Search index timeout (1 minute)
  • Another entity type showing the same infrastructure issue

3. Playwright test failures (flaky, unrelated):

  • EntityVersionPages.spec.ts:82: Element visibility issue
  • AuditLogs.spec.ts:355: Element visibility timeout
  • Custom properties, Data Products, Teams, Domains: 14+ flaky tests

4. Previous failures from earlier commits (context):

  • ExploreTree.spec.ts:439: ✅ FIXED in commit 212e3cc (case sensitivity issue)
  • LineageResourceIT (lines 224, 781): OpenSearch version conflicts
  • TableResourceIT: Search index timeouts
  • WebAnalyticEventResourceIT: Data cleanup issues

Details

PR Changes:

  • ChartRepository.java - Fixed chart service assignment during bulk reindex
  • ChartResourceIT.java - Backend test for chart service
  • ExploreTree.spec.ts - UI test (✅ FIXED in commit 212e3cc)

Pattern Analysis:
Multiple entity types now showing search index timeouts:

  • Domain (7 different test methods)
  • Chart (new test from PR)
  • Glossary/GlossaryTerm
  • Topic (NEW)
  • Table (earlier runs)

All have identical error patterns: "Wait for search to reflect update" with 1 minute timeout. This confirms a systemic Elasticsearch async indexing performance issue in the test infrastructure, not PR-specific logic bugs.

Summary:

  • ExploreTree.spec.ts test: ✅ FIXED (case sensitivity resolved)
  • TopicResourceIT: Flaky search index timeout (new entity type, unrelated)
  • DomainResourceIT (7 different lines): Highly flaky search index timeouts (unrelated)
  • ChartResourceIT.java:1: Flaky search index timeout (infrastructure, not PR logic)
  • GlossaryTermResourceIT, GlossaryResourceIT, TableResourceIT: Flaky search index timeouts (unrelated)
  • LineageResourceIT: Flaky version conflicts (unrelated)
  • EntityVersionPages.spec.ts, AuditLogs.spec.ts: Flaky visibility issues (unrelated)
  • All failures are infrastructure/timing issues, unrelated to PR's Chart service fix
Code Review ✅ Approved

Clean, well-targeted bug fix that removes the faulty batch service assignment and relies on the existing correct per-entity resolution in fetchAndSetDefaultService. Good regression test coverage and Playwright test improvements.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

@sonarqubecloud
Copy link
Copy Markdown

@pmbrull pmbrull merged commit fa1dde8 into main Feb 20, 2026
36 of 42 checks passed
@pmbrull pmbrull deleted the fix/chart-service-bulk-load- branch February 20, 2026 16:38
pmbrull pushed a commit that referenced this pull request Feb 20, 2026
* Fix charts getting wrong service during reindex

* address feedback from gitar bot

* Fix explore tree spec

---------

Co-authored-by: Aniket Katkar <aniketkatkar97@gmail.com>
@github-actions
Copy link
Copy Markdown
Contributor

Failed to cherry-pick changes to the 1.11.11 branch.
Please cherry-pick the changes manually.
You can find more details here.

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 To release Will cherry-pick this PR into the release branch UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants