Perf/redis cache metrics and indexes

[harshach]

Describe your changes:

Fixes

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

• Resilience improvements:
  • Added DeadlockRetry utility using resilience4j to handle database transaction deadlocks with exponential backoff.
  • Refactored TestCaseRepository.addAllTestCasesToLogicalTestSuite to use DeadlockRetry for more robust bulk insertions during concurrent operations.

_{This will update automatically on new commits.}

[github-actions]

The Java checkstyle failed.

Please run mvn spotless:apply in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.

[Copilot]

awaitAll ignores the boolean result from LettuceFutures.awaitAll(...). If the await times out or some futures complete exceptionally, warmup/batch writes can partially fail but the caller will still record writes and proceed as if successful. Capture the return value and/or inspect futures for exceptions; if not all completed successfully, log and treat the batch as failed (and record an error) so warmup correctness/observability is preserved.

Suggested change

	LettuceFutures.awaitAll(timeoutMs, TimeUnit.MILLISECONDS, futures.toArray(new RedisFuture[0]));
	RedisFuture<?>[] futureArray = futures.toArray(new RedisFuture[0]);
	boolean completed = LettuceFutures.awaitAll(timeoutMs, TimeUnit.MILLISECONDS, futureArray);
	int failedFutures = 0;
	for (RedisFuture<?> future : futureArray) {
	if (!future.isDone() || future.isCancelled()) {
	failedFutures++;
	continue;
	}
	try {
	future.get();
	} catch (InterruptedException e) {
	Thread.currentThread().interrupt();
	LOG.error("Interrupted while waiting for Redis pipeline futures");
	throw new IllegalStateException("Interrupted while waiting for Redis pipeline futures", e);
	} catch (Exception e) {
	failedFutures++;
	LOG.error("Redis pipeline future completed exceptionally", e);
	}
	}
	if (!completed || failedFutures > 0) {
	LOG.error(
	"Redis pipeline batch did not complete successfully (completed={}, failedFutures={}, totalFutures={}, timeoutMs={})",
	completed,
	failedFutures,
	futureArray.length,
	timeoutMs);
	throw new IllegalStateException(
	String.format(
	"Redis pipeline batch failed (completed=%s, failedFutures=%d, totalFutures=%d)",
	completed, failedFutures, futureArray.length));
	}

[Copilot]

The comment says this invalidation happens before scheduling an async writeThroughCache, and later says "Kick off the async repopulate", but writeThroughCache is now synchronous. This is misleading for future maintainers trying to reason about cache/DB ordering—please update the comment(s) to reflect the current synchronous behavior (or reintroduce async if intended).

[Copilot]

executeWithDeadlockRetry retries immediately after a deadlock without any backoff/jitter. In practice this can re-hit the same lock contention pattern and exhaust retries quickly. Consider adding a short exponential backoff (and maybe jitter) between attempts, and include the attempt count + sleep duration in the log to aid diagnosing CI flakiness.

[gitar-bot]

💡 Quality: Pipeline errors swallowed without logging root cause

In awaitAll, when f.get() throws a non-InterruptedException (line 286-288), the exception is silently counted but not logged. When the method later throws IllegalStateException with the count summary, operators have no way to diagnose why individual commands failed (e.g., NOSCRIPT, OOM, connection reset). Consider logging the first exception or attaching it as a suppressed exception to the thrown ISE.

Suggested fix:

} catch (Exception e) {
  if (failed == 0) {
    LOG.warn("Redis pipeline command failed", e);
  }
  failed++;
}

_{Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion}

[gitar-bot]

Code Review 👍 Approved with suggestions 6 resolved / 7 findings

Refactors Redis cache metrics and indexing logic to resolve database compatibility, thread blocking, and race condition issues. Update the awaitAll implementation to log pipeline error root causes.

💡 Quality: Pipeline errors swallowed without logging root cause

📄 openmetadata-service/src/main/java/org/openmetadata/service/cache/RedisCacheProvider.java:286-288 📄 openmetadata-service/src/main/java/org/openmetadata/service/cache/RedisCacheProvider.java:290-295

In awaitAll, when f.get() throws a non-InterruptedException (line 286-288), the exception is silently counted but not logged. When the method later throws IllegalStateException with the count summary, operators have no way to diagnose why individual commands failed (e.g., NOSCRIPT, OOM, connection reset). Consider logging the first exception or attaching it as a suppressed exception to the thrown ISE.

Suggested fix

} catch (Exception e) {
  if (failed == 0) {
    LOG.warn("Redis pipeline command failed", e);
  }
  failed++;
}

✅ 6 resolved

✅ Bug: listIdFqnByPrefixHash uses MySQL-only JSON syntax, breaks PostgreSQL

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityDAO.java:160-167
The new @SqlQuery for listIdFqnByPrefixHash uses JSON_UNQUOTE(JSON_EXTRACT(json, '$.fullyQualifiedName')) which is MySQL-specific. PostgreSQL requires json->>'fullyQualifiedName' syntax. Other methods in the same interface (e.g. updateWithVersion at line 137) correctly use @ConnectionAwareSqlUpdate / @ConnectionAwareSqlQuery with separate MySQL and PostgreSQL variants. This query will fail at runtime for any PostgreSQL deployment that triggers a domain or data-product rename cascade.

✅ Performance: waitForConcurrentLoad busy-polls with Thread.sleep on request thread

📄 openmetadata-service/src/main/java/org/openmetadata/service/cache/CachedReadBundle.java:92-106
CachedReadBundle.waitForConcurrentLoad (lines 92-107) blocks the calling Jetty request thread with a Thread.sleep polling loop for up to loadLockWaitMs (default 200ms). Under cold-cache scenarios with many distinct entities, each concurrent request blocks a thread pool slot. With the default Jetty thread pool (typically 200 threads), a burst of ~200 simultaneous cold-cache reads for different entities would exhaust the thread pool for 200ms, causing request queuing.

The 200ms default mitigates the blast radius, but this is still a blocking wait on a request thread. Consider falling through to DB immediately instead of polling (i.e., skip the wait entirely and just let multiple callers load in parallel — the Redis write is idempotent), or use a local Striped/CountDownLatch so the wait doesn't require Redis round-trips.

✅ Edge Case: buildBundleDto returning null leaves single-flight waiters spinning

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:1656-1670 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:1714
When the load-lock holder's buildBundleDto returns null (line 1714 — all relations empty, no tags, no certification), it never populates the cache, but waiters from waitForConcurrentLoad are still polling for up to loadLockWaitMs. They'll all time out and fall through to their own DB loads. While this is safe (they fall back to DB), it defeats the single-flight purpose for entities with no cacheable bundle data. These entities will always trigger the lock + wait + timeout cycle on every cold read.

Consider writing a sentinel/empty-marker entry when buildBundleDto returns null so waiters get a fast "nothing to cache" signal, or skip tryAcquireLoadLock entirely for entity types known to have empty bundles.

✅ Bug: CacheInvalidationPubSub duplicates RedisURI parsing logic

📄 openmetadata-service/src/main/java/org/openmetadata/service/cache/CacheInvalidationPubSub.java:136-150 📄 openmetadata-service/src/main/java/org/openmetadata/service/cache/RedisCacheProvider.java:49-63
CacheInvalidationPubSub.buildRedisURI (lines 136-165) duplicates the same URL parsing and auth logic from RedisCacheProvider.buildRedisURI (lines 49-84). This creates a maintenance risk — if one is updated (e.g., to support Redis Sentinel/Cluster or a new auth type), the other may be missed. Consider extracting a shared utility method, or having the pub/sub reuse the existing RedisClient from CacheProvider.

✅ Edge Case: pipelineHset timeout may be excessive for large batches

📄 openmetadata-service/src/main/java/org/openmetadata/service/cache/RedisCacheProvider.java:311-315
In awaitAll (line 312), the timeout is commandTimeoutMs * 10 applied per-future sequentially. For a batch of 1000 HSET+EXPIRE pairs (2000 futures), the theoretical worst case is 2000 × commandTimeoutMs × 10 = 600 seconds with the default 300ms timeout. In practice Redis pipelines complete fast, but a hung connection could block the warmup thread for a very long time. Consider using LettuceFutures.awaitAll(timeout, futures) which applies a single timeout to the entire batch.

...and 1 more resolved from earlier reviews

🤖 Prompt for agents

Code Review: Refactors Redis cache metrics and indexing logic to resolve database compatibility, thread blocking, and race condition issues. Update the `awaitAll` implementation to log pipeline error root causes.

1. 💡 Quality: Pipeline errors swallowed without logging root cause
   Files: openmetadata-service/src/main/java/org/openmetadata/service/cache/RedisCacheProvider.java:286-288, openmetadata-service/src/main/java/org/openmetadata/service/cache/RedisCacheProvider.java:290-295

   In `awaitAll`, when `f.get()` throws a non-InterruptedException (line 286-288), the exception is silently counted but not logged. When the method later throws `IllegalStateException` with the count summary, operators have no way to diagnose *why* individual commands failed (e.g., NOSCRIPT, OOM, connection reset). Consider logging the first exception or attaching it as a suppressed exception to the thrown ISE.

   Suggested fix:
   } catch (Exception e) {
     if (failed == 0) {
       LOG.warn("Redis pipeline command failed", e);
     }
     failed++;
   }

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact

gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 5 comments.

[Copilot]

success/failed are cumulative counters across all pages, but updateEntityStats(...) treats its arguments as deltas and adds them to the stored totals. Calling updateEntityStats(entityType, success, failed) each loop will over-count (1st page adds N, 2nd page adds 2N, etc.). Track per-page counts (reset to 0 each iteration) or pass (pageSuccess, pageFailed) instead.

[Copilot]

isDistributedCacheEnabled() checks CacheBundle.getCachedEntityDao() != null, which can be true even when Redis initialization failed or the cache is unhealthy. In that case, REST GETs will still consult per-process caches and can serve stale data in multi-instance setups. Consider gating this on CacheBundle.getCacheProvider().available() (and/or CacheBundle.getCacheConfig().provider == redis) so cached reads are only enabled when the distributed cache is actually usable.

[Copilot]

The Javadoc states resilience4j “schedules the delay on its own executor, so the request thread yields instead of blocking on Thread.sleep”, but Retry.executeSupplier(...) performs synchronous retries and (in resilience4j-retry 2.3.0) waits by sleeping the calling thread. Please adjust the comment to reflect the actual behavior, or switch to an async retry API if non-blocking backoff is required.

Suggested change

	* <p>Backoff: resilience4j schedules the delay on its own executor, so the request thread yields
	* instead of blocking on {@link Thread#sleep}. Exponential base 50 ms × 2^(attempt-1) with 50%
	* jitter — attempt 1 ≈ 25-75 ms, attempt 2 ≈ 50-150 ms, attempt 3 ≈ 100-300 ms.
	* <p>Backoff: retries are synchronous when invoked via {@link Retry#executeSupplier(Supplier)}, so
	* the calling thread waits between attempts according to the configured interval. Exponential base
	* 50 ms × 2^(attempt-1) with 50% jitter — attempt 1 ≈ 25-75 ms, attempt 2 ≈ 50-150 ms, attempt
	* 3 ≈ 100-300 ms.

[Copilot]

loadLock is only released in the finally block at the end of the method, but the lock is acquired much earlier. If any exception is thrown during relationship/tag/vote/extension fetch (before the final try/finally), the lock will never be unlocked, causing subsequent requests for the same (type,id) on this instance to deadlock. Wrap the whole post-lock code path in a try/finally that always unlocks, or unlock immediately if the re-check finds the bundle already populated.

Suggested change

	try (var ignored = phase("readBundleCacheGet")) {
	initialDto = bundleCache.get(entityType, entity.getId());
	}
	try {
	try (var ignored = phase("readBundleCacheGet")) {
	initialDto = bundleCache.get(entityType, entity.getId());
	}
	} catch (RuntimeException e) {
	loadLock.unlock();
	loadLock = null;
	throw e;
	} catch (Error e) {
	loadLock.unlock();
	loadLock = null;
	throw e;
	}
	if (initialDto != null) {
	loadLock.unlock();
	loadLock = null;
	}

[Copilot]

DeadlockRetry.execute(...) is invoked from within a method annotated with @Transaction. That means each retry re-runs only the lambda inside the same JDBI transaction scope, so it won't start a fresh transaction per attempt (and may run on an already-rolled-back handle after a deadlock). To match the intent (retry the whole transaction), move the retry wrapper outside the @Transaction boundary (e.g., public wrapper calls DeadlockRetry.execute(() -> addAllTestCasesToLogicalTestSuiteTxn(...)) where the inner method is @Transaction).

[sonarqubecloud]

Quality Gate passed for 'open-metadata-ingestion'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud