Add Redis client utility and corresponding tests - #3172
Open
Thenujan-Nagaratnam wants to merge 3 commits into
Open
Add Redis client utility and corresponding tests#3172Thenujan-Nagaratnam wants to merge 3 commits into
Thenujan-Nagaratnam wants to merge 3 commits into
Conversation
Thenujan-Nagaratnam
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Induwara04,
Krishanx92,
PasanT9,
Piumal1999,
RakhithaRR,
Tharsanan1,
Thushani-Jayasekera,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
lasanthaS,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub,
tharindu1st and
thivindu
as code owners
August 7, 2026 04:59
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a process-wide Redis client registry in ChangesRedis client registry
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PolicyCallSite
participant GetOrCreateRedisClient
participant RedisClientRegistry
participant RedisServer
PolicyCallSite->>GetOrCreateRedisClient: request client with Redis options
GetOrCreateRedisClient->>RedisClientRegistry: look up configuration key
alt client is not registered
GetOrCreateRedisClient->>RedisServer: ping new client with timeout
RedisServer-->>GetOrCreateRedisClient: ping result
GetOrCreateRedisClient->>RedisClientRegistry: register shareable client
else client is registered
RedisClientRegistry-->>GetOrCreateRedisClient: return shared client
end
GetOrCreateRedisClient-->>PolicyCallSite: client, created status, ping error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@sdk/core/utils/redisclient/redisclient_test.go`:
- Around line 123-135: Update TestHashPassword to replace the
identical-expression comparison with an assertion that
hashRedisPassword("secret") equals the known SHA-256 hexadecimal digest for
“secret”; retain the other password behavior checks unchanged.
In `@sdk/core/utils/redisclient/redisclient.go`:
- Around line 79-91: Update the client-creation flow around the redisClients
mutex: register the newly created client while the mutex is held, then release
the lock before creating the timeout context and calling c.Ping(ctx). Preserve
the existing-client fast path and ensure deferred unlock behavior does not keep
the registry locked during network I/O.
- Around line 68-77: Update the redisConnKey construction in
GetOrCreateRedisClient to distinguish clients with different TLSConfig,
Protocol, and credential-provider settings. Add safe, comparable fingerprints
for these options, or bypass registry reuse when an option cannot be safely
fingerprinted; preserve existing password handling and ensure differing
connection behavior cannot share a cached client.
🪄 Autofix
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: 557670c7-3f35-4077-be82-c233c67f2606
⛔ Files ignored due to path filters (1)
sdk/core/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
sdk/core/go.modsdk/core/utils/redisclient/redisclient.gosdk/core/utils/redisclient/redisclient_test.go
…tion handling with TLS and credentials provider
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@sdk/core/utils/redisclient/redisclient_test.go`:
- Line 187: Update the deferred listener cleanup around ln.Close to explicitly
handle or discard its returned error, and update the slow-call results at the
referenced synchronization-test locations with blank assignments when
intentionally unused. Ensure all affected calls satisfy errcheck without
changing the test behavior.
- Around line 188-213: The test’s time.Sleep does not guarantee the slow Redis
client goroutine has reached Ping before the fast call is measured. Add a
synchronization channel signaled immediately after ln.Accept() succeeds, then
wait for that signal with a bounded test timeout before creating the unrelated
client, preserving the existing slow-client setup and cleanup behavior.
🪄 Autofix
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: a79bb6e9-39aa-494e-8dd9-268f1d8ae02e
📒 Files selected for processing (2)
sdk/core/utils/redisclient/redisclient.gosdk/core/utils/redisclient/redisclient_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/core/utils/redisclient/redisclient.go
…re accurate timeout signaling
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a new shared Redis client utility to the SDK core, ensuring that identical Redis connection configurations reuse a single
*redis.Clientinstance process-wide, which prevents connection pool leaks and excessive Redis connections. It also adds comprehensive tests for this utility and updates dependencies to support the new functionality.New Redis client sharing utility:
redisclientpackage withGetOrCreateRedisClient, which maintains a process-wide registry of shared*redis.Clientinstances keyed by connection configuration. This ensures connection pools are reused for identical settings and prevents resource leaks. (sdk/core/utils/redisclient/redisclient.go)sdk/core/utils/redisclient/redisclient.go)Testing:
sdk/core/utils/redisclient/redisclient_test.go)Dependency management:
go.modto include new dependencies:go-redis/v9for Redis support andminiredis/v2for in-memory Redis testing, along with related indirect dependencies. (sdk/core/go.mod)