LEGLINK-857: Improvements to avoid Redis timeouts - #1770
Conversation
…pool instead of a multiplexer
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRedis integration was migrated to StackExchange.Redis.Extensions with configurable connection pools. Cache reads, writes, and deletes now use asynchronous cancellation-aware APIs, FHIR and normalization callers await them, distributed semaphore acquisition is asynchronous, and related tests and lockfiles were updated. ChangesRedis foundation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
DotNet/Shared/Application/Health/ResourceCacheHealthCheck.cs (1)
33-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused health-check tests for every cache mode.
Cover Redis, ABS, and Hybrid, including Redis failure preventing the Blob check and cancellation/timeout failures. Mock
IRedisDatabaseand Blob interactions; do not contact external services.As per path instructions, “If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test,” using Moq for external communication.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DotNet/Shared/Application/Health/ResourceCacheHealthCheck.cs` around lines 33 - 47, Add focused unit tests for ResourceCacheHealthCheck covering Redis, ABS, and Hybrid modes, with each switch branch exercised. Mock IRedisDatabase and Blob interactions using Moq, verify Hybrid skips Blob checks when Redis is unhealthy, and cover cancellation/timeout failure results without contacting external services.Source: Path instructions
DotNet/Shared/Application/Extensions/ResourceCacheExtensions.cs (1)
57-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover each cache-registration branch with XUnit tests.
Verify Redis resolves
RedisResourceCacheandIRedisDatabase, ABS resolves without Redis, and Hybrid resolves both keyed implementations. Use Moq/configuration objects without opening Redis or Blob Storage connections.As per path instructions, “If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test,” and unit tests must contain no network activity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DotNet/Shared/Application/Extensions/ResourceCacheExtensions.cs` around lines 57 - 79, Add XUnit coverage for each `ResourceCacheType` branch in the registration method: verify Redis resolves `RedisResourceCache` and `IRedisDatabase`, ABS resolves `ABSResourceCache` without Redis services, and Hybrid resolves both keyed `RedisResourceCache` and `ABSResourceCache`. Build the tests with mocked dependencies and configuration objects so service registration is validated without opening Redis or Blob Storage connections.Source: Path instructions
DotNet/Shared/Application/Models/Configs/DistributedLockSettings.cs (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject invalid Redis pool sizes at startup.
PoolSizeis configuration-bound and passed through unvalidated. Reject<= 0deterministically, and add focused XUnit coverage for the default, zero, and negative cases without resolving Redis.Proposed fix
+ if (distributedLockSettings.PoolSize <= 0) + { + throw new ArgumentOutOfRangeException( + nameof(distributedLockSettings.PoolSize), + "DistributedLockSettings:PoolSize must be greater than zero."); + } + services.AddStackExchangeRedisExtensions<SystemTextJsonSerializer>(new StackExchange.Redis.Extensions.Core.Configuration.RedisConfigurationAs per path instructions, “Logic that depends on service or interface configuration” should have unit tests, and unit tests must not perform network activity.
Also applies to: 100-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DotNet/Shared/Application/Models/Configs/DistributedLockSettings.cs` at line 15, Validate DistributedLockSettings.PoolSize during configuration/startup and reject any value less than or equal to zero deterministically before Redis resolution or connection attempts. Add focused XUnit tests covering the default value, zero, and negative values, ensuring the tests exercise validation only and perform no network activity.Source: Path instructions
DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs (1)
101-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd focused XUnit coverage for the modified cache branches.
Use Moq or narrow wrappers; do not use Redis or Blob network calls in unit tests.
DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs#L101-L170: cover connected/unavailable Redis, missing or invalid memory values, threshold fallback, and exceptions.DotNet/Shared/Application/Services/ResourceCache/ABSResourceCache.cs#L46-L65: cover ids-blob present and absent paths.DotNet/Shared/Application/Services/ResourceCache/ABSResourceCache.cs#L116-L121: cover cache-hit and cache-miss reads.DotNet/DataAcquisition.Domain/Application/Services/FhirApi/FhirApiService.cs#L436-L442: cover cacheable and non-cacheable resource inputs.DotNet/DataAcquisition.Domain/Application/Services/ReferenceResourceService.cs#L537-L546: cover cacheable and non-cacheable cached-reference inputs.As per path instructions, “If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test,” and “No network activity … should appear in unit tests. Recommend using mocks (via Moq) for any external communication.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs` around lines 101 - 170, Add focused XUnit tests using Moq or narrow wrappers, with no Redis, Blob, or other network calls. In DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs:101-170, cover connected and unavailable Redis, missing or invalid memory values, threshold-based ABS/Redis selection, and exception fallback. In DotNet/Shared/Application/Services/ResourceCache/ABSResourceCache.cs:46-65 and :116-121, cover ids-blob present/absent and cache-hit/cache-miss reads. In DotNet/DataAcquisition.Domain/Application/Services/FhirApi/FhirApiService.cs:436-442 and DotNet/DataAcquisition.Domain/Application/Services/ReferenceResourceService.cs:537-546, cover cacheable and non-cacheable inputs.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DotNet/Shared/Application/Extensions/ResourceCacheExtensions.cs`:
- Around line 92-95: Update resource cache initialization around
ValidateRedisSettings and the StackExchangeRedisExtensions registration to
reject RedisSettings.PoolSize values less than or equal to zero before
constructing RedisConfiguration, while preserving the default and positive
configured values. Extend the focused XUnit coverage to verify the default pool
size, a positive override, and startup failure for zero and negative values.
- Around line 89-90: Update the Redis configuration setup around
ConfigurationOptions.Parse so redisSettings.Password only overrides the parsed
password when it is explicitly non-null/non-empty; otherwise preserve the
credential embedded in the connection string. Keep the existing configuration
flow and Password assignment behavior for an explicitly configured ResourceCache
Redis password.
In `@DotNet/Shared/Application/Services/ResourceCache/ABSResourceCache.cs`:
- Around line 118-120: Sanitize both the cache-derived arguments, cacheKey and
the GetBlobKey(cacheKey) result, before passing them to _logger.LogWarning in
the ABS blob-not-found path. Preserve the existing warning message and settings
argument, and ensure every logged value is sanitized at the call site.
---
Nitpick comments:
In `@DotNet/Shared/Application/Extensions/ResourceCacheExtensions.cs`:
- Around line 57-79: Add XUnit coverage for each `ResourceCacheType` branch in
the registration method: verify Redis resolves `RedisResourceCache` and
`IRedisDatabase`, ABS resolves `ABSResourceCache` without Redis services, and
Hybrid resolves both keyed `RedisResourceCache` and `ABSResourceCache`. Build
the tests with mocked dependencies and configuration objects so service
registration is validated without opening Redis or Blob Storage connections.
In `@DotNet/Shared/Application/Health/ResourceCacheHealthCheck.cs`:
- Around line 33-47: Add focused unit tests for ResourceCacheHealthCheck
covering Redis, ABS, and Hybrid modes, with each switch branch exercised. Mock
IRedisDatabase and Blob interactions using Moq, verify Hybrid skips Blob checks
when Redis is unhealthy, and cover cancellation/timeout failure results without
contacting external services.
In `@DotNet/Shared/Application/Models/Configs/DistributedLockSettings.cs`:
- Line 15: Validate DistributedLockSettings.PoolSize during
configuration/startup and reject any value less than or equal to zero
deterministically before Redis resolution or connection attempts. Add focused
XUnit tests covering the default value, zero, and negative values, ensuring the
tests exercise validation only and perform no network activity.
In `@DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs`:
- Around line 101-170: Add focused XUnit tests using Moq or narrow wrappers,
with no Redis, Blob, or other network calls. In
DotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.cs:101-170,
cover connected and unavailable Redis, missing or invalid memory values,
threshold-based ABS/Redis selection, and exception fallback. In
DotNet/Shared/Application/Services/ResourceCache/ABSResourceCache.cs:46-65 and
:116-121, cover ids-blob present/absent and cache-hit/cache-miss reads. In
DotNet/DataAcquisition.Domain/Application/Services/FhirApi/FhirApiService.cs:436-442
and
DotNet/DataAcquisition.Domain/Application/Services/ReferenceResourceService.cs:537-546,
cover cacheable and non-cacheable inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aa6f24a-d267-4183-8bbf-cdce61bc7a97
📒 Files selected for processing (47)
DotNet/Account/appsettings.Docker.jsonDotNet/Account/packages.lock.jsonDotNet/Admin.BFF/appsettings.Docker.jsonDotNet/Admin.BFF/packages.lock.jsonDotNet/Audit/packages.lock.jsonDotNet/Automation.Link/packages.lock.jsonDotNet/Automation.UI/packages.lock.jsonDotNet/Census/packages.lock.jsonDotNet/DataAcquisition.AcquisitionWorker/appsettings.Docker.jsonDotNet/DataAcquisition.AcquisitionWorker/packages.lock.jsonDotNet/DataAcquisition.Domain/Application/Services/FhirApi/Commands/ReadFhirCommand.csDotNet/DataAcquisition.Domain/Application/Services/FhirApi/Commands/SearchFhirCommand.csDotNet/DataAcquisition.Domain/Application/Services/FhirApi/FhirApiService.csDotNet/DataAcquisition.Domain/Application/Services/LocationMappingService.csDotNet/DataAcquisition.Domain/Application/Services/ReferenceResourceService.csDotNet/DataAcquisition.Domain/packages.lock.jsonDotNet/DataAcquisition/appsettings.Docker.jsonDotNet/DataAcquisition/packages.lock.jsonDotNet/LinkSdk/packages.lock.jsonDotNet/Normalization/Listeners/ResourcesAcquiredListener.csDotNet/Normalization/packages.lock.jsonDotNet/QueryDispatch/packages.lock.jsonDotNet/Report/packages.lock.jsonDotNet/ServiceTests/IntegrationTests/DataAcquisition/AcquisitionProcessingBackgroundServiceTests.csDotNet/ServiceTests/IntegrationTests/Normalization/NormalizationIntegrationTestFixture.csDotNet/ServiceTests/UnitTests/DataAcquisition/FhirApiServiceTests.csDotNet/ServiceTests/UnitTests/DataAcquisition/SearchFhirCommandTests.csDotNet/ServiceTests/UnitTests/DataAcquisition/Services/LocationMappingServiceTests.csDotNet/ServiceTests/packages.lock.jsonDotNet/Shared/Application/Extensions/ExternalServices/RedisCacheExtension.csDotNet/Shared/Application/Extensions/ResourceCacheExtensions.csDotNet/Shared/Application/Health/ResourceCacheHealthCheck.csDotNet/Shared/Application/Interfaces/IResourceCache.csDotNet/Shared/Application/Models/Configs/DistributedLockSettings.csDotNet/Shared/Application/Models/Configs/ResourceCacheSettings.csDotNet/Shared/Application/Services/ResourceCache/ABSResourceCache.csDotNet/Shared/Application/Services/ResourceCache/HybridResourceCache.csDotNet/Shared/Application/Services/ResourceCache/RedisResourceCache.csDotNet/Shared/Shared.csprojDotNet/Shared/packages.lock.jsonDotNet/Submission.Data/packages.lock.jsonDotNet/Submission/packages.lock.jsonDotNet/Tenant/packages.lock.jsonDotNet/Terminology/packages.lock.jsonTests/BackendE2ETests/packages.lock.jsonapp-config.yamldocker-compose.yml
👮 Files not reviewed due to content moderation or server errors (9)
- DotNet/Audit/packages.lock.json
- DotNet/Automation.Link/packages.lock.json
- DotNet/Automation.UI/packages.lock.json
- DotNet/Census/packages.lock.json
- DotNet/DataAcquisition.AcquisitionWorker/packages.lock.json
- DotNet/DataAcquisition.Domain/Application/Services/FhirApi/Commands/ReadFhirCommand.cs
- DotNet/DataAcquisition.Domain/Application/Services/FhirApi/Commands/SearchFhirCommand.cs
- DotNet/DataAcquisition.Domain/packages.lock.json
- DotNet/DataAcquisition/packages.lock.json
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
DataAcquisition and AcquisitionWorker run two different redis configurations — GeneralStartupExtensions.cs:90 (RegisterRedis()) then :102 (AddResourceCache). Normalization runs only the second. So the last one wins. |
|
Did you mean to downgrade redist to 2.8.24 from 2.13.17? All of the package lock files show a downgrade |
Add support for using an existing shared Redis connection when registering the resource cache. ResourceCacheExtensions.AddResourceCache now accepts useExistingRedisConnection and uses RegisterOrUseRedisConnection to either validate an existing IRedisDatabase registration or register a new connection. GeneralStartupExtensions passes the flag and conditionally registers Redis. Added unit tests (ResourceCacheExtensionsTests) covering hybrid/redis/abs scenarios and error when existing connection is missing. Removed ResourceCache Redis connection/password from appsettings and docker-compose to avoid duplicate registrations. Updated app-config.yaml and environment config files (app-config.*.json) to clarify Redis usage and narrow some consumer tags.
Bump central package versions in Directory.Packages.props: upgrade StackExchange.Redis to 3.1.11, StackExchange.Redis.Extensions.* to 13.0.1, Microsoft.Extensions.Caching.StackExchangeRedis to 10.0.10, and several Microsoft.Extensions packages (DependencyInjection, Options, Primitives, Telemetry, etc.) to 10.0.10/10.0.5 where applicable. Regenerated packages.lock.json files across projects to reflect the new dependency graph and updated transitive runtime packages. Also add an explicit StackExchange.Redis PackageReference to DotNet/Shared/Shared.csproj so shared projects reference the upgraded Redis client.
Add PooledRedisDistributedSemaphoreProvider that pulls an IRedisDatabase and creates a RedisDistributedSynchronizationProvider per semaphore. Update DI to register the pooled provider. Change RedisResourceCache to depend on IRedisDatabase and obtain Database per operation instead of holding a single IDatabase. Add unit tests verifying pooled Database usage for semaphores and cache deletions. Files: PooledRedisDistributedSemaphoreProvider.cs, DistributedLockSettings.cs (modified), RedisResourceCache.cs (modified), plus two new unit tests.
Updated AddResourceCache so that it can reuse an existing Redis connection. DA and DAW now reuse the existing connection for ResourceCache. Normalization still uses the original ResourceCache connection. This allows us to use different connections for RegisterRedis and AddResourceCache if desired.
No. It was changed to a transitive dependency, so it restored the minimum required version. I made it explicit and updated all the Redis packages to the latest. |
|
Do the connection strings for Redis need to be updated in these files (they are from my previous changes)?
|
Appends Redis client options (abortConnect=false, resolveDns=true, allowAdmin=true) to ConnectionStrings:Redis and ResourceCache:Redis:ConnectionString in dev, qa, qa2, and test app-config JSON files. This enables DNS resolution for managed Redis endpoints, allows admin commands, and prevents early aborts on connection failures.
4 participants
🛠️ Description of Changes
🧪 Testing Performed
Unit tests and local integration tests.
🧑🔬 Unit Testing
📓 Documentation Updated
N/A
Summary by CodeRabbit