Skip to content

RedisConnection class enhancements & refactoring#10732

Merged
julianbrost merged 4 commits intomasterfrom
icingadb-efficient-queue-helpers
Mar 3, 2026
Merged

RedisConnection class enhancements & refactoring#10732
julianbrost merged 4 commits intomasterfrom
icingadb-efficient-queue-helpers

Conversation

@yhabteab
Copy link
Copy Markdown
Member

@yhabteab yhabteab commented Feb 24, 2026 •
edited
Loading

This PR was originally part of #10619 but was split out to make it easier to review as that PR has become quite large.
The changes are trivial and mostly related to code cleanup and refactoring to make the RedisConnection class more useful and easier to maintain. As I mentioned, these changes are necessary for the other PR this one was originally part of.

@yhabteab yhabteab requested a review from julianbrost February 24, 2026 15:26
@cla-bot cla-bot Bot added the cla/signed label Feb 24, 2026
@yhabteab yhabteab changed the title Icingadb efficient queue helpers RedisConnection class enhancements & refactoring Feb 24, 2026
@yhabteab yhabteab mentioned this pull request Feb 24, 2026
Merged
@yhabteab yhabteab force-pushed the icingadb-efficient-queue-helpers branch from ff67cd6 to ae11a10 Compare February 24, 2026 17:11
@yhabteab
Copy link
Copy Markdown
Member Author

yhabteab commented Feb 24, 2026

Dropped one commit from this PR as requested from Julian, and re-added it to the PR instead.

@julianbrost julianbrost added this to the 2.16.0 milestone Feb 25, 2026
julianbrost
julianbrost reviewed Feb 25, 2026
View reviewed changes
Comment thread lib/icingadb/redisconnection.hpp Outdated
Comment thread lib/icingadb/redisconnection.cpp Outdated
Comment thread lib/icingadb/redisconnection.cpp Outdated
@yhabteab yhabteab force-pushed the icingadb-efficient-queue-helpers branch from ae11a10 to fd0f2a4 Compare February 25, 2026 11:18
@yhabteab yhabteab requested a review from julianbrost February 25, 2026 11:19
julianbrost
julianbrost reviewed Feb 25, 2026
View reviewed changes
Comment thread lib/icingadb/redisconnection.cpp
Comment thread lib/icingadb/redisconnection.cpp Outdated
Comment thread lib/icingadb/redisconnection.cpp Outdated
Comment thread lib/icingadb/redisconnection.cpp Outdated
@yhabteab yhabteab force-pushed the icingadb-efficient-queue-helpers branch from fd0f2a4 to 536bac4 Compare February 25, 2026 14:13
@yhabteab yhabteab requested a review from julianbrost February 25, 2026 14:17
@yhabteab yhabteab force-pushed the icingadb-efficient-queue-helpers branch from 8300634 to c3612b3 Compare March 2, 2026 12:36
@yhabteab
Copy link
Copy Markdown
Member Author

yhabteab commented Mar 2, 2026

Simplified the reconnect handling of the redis connection class and get rid of the confusing m_Connecting flag by using a persistent Connect coroutine instead of temporary ones. See c3612b3 for details.

@julianbrost
Copy link
Copy Markdown
Member

julianbrost commented Mar 2, 2026

My motivation for asking you to create this PR was so that we can get the uncontroversial changes out of the way, so that for example, one doesn't have to look over the changes from 7d96272 over and over again when looking at #10619.

Just closing connections that are no longer needed sounded uncontroversial, but it looks like it opened another can of worms in RedisConnection. Yes, keeping these connection open when they are no longer used isn't nice and worth fixing, but it this actually a requirement for any of the other changes? If not, can we please treat it as the separate issue it is, i.e. with its own PR so that we can first move on with the thing we actually wanted to do?

@yhabteab yhabteab force-pushed the icingadb-efficient-queue-helpers branch from c3612b3 to 2136dfc Compare March 2, 2026 13:42
@yhabteab
Copy link
Copy Markdown
Member Author

yhabteab commented Mar 2, 2026

ust closing connections that are no longer needed sounded uncontroversial, but it looks like it opened another can of worms in RedisConnection.

Actually, it was quite simple and straightforward and it didn't open anything and none of that was a problem at all.
I just added the last commit (which btw is just a trivial one) to further simplify the RedisConnection class as you and myself were both confused about the usages of the m_Connecting flag.

So, dropped all that stuff anyway as requested for whatever reason.

julianbrost
julianbrost reviewed Mar 2, 2026
View reviewed changes
Comment thread lib/icingadb/redisconnection.hpp Outdated
Comment thread lib/icingadb/redisconnection.cpp Outdated
yhabteab added 4 commits March 2, 2026 15:34
@yhabteab
RedisConnection: introduce & use RedisConnInfo helper struct
4d4650b
@yhabteab
Allow to track query stats in child Redis connections
e0c2391 
So that when we want the query stats of this specific connection we can
easily get them, since the session leader contains the aggregated stats
of all its children.
@yhabteab
RedisConnection: drop superfluous GetConnected method
dd5289b
@yhabteab
ObjectLock: allow to TryLock the mutex
c1763e4
@yhabteab yhabteab force-pushed the icingadb-efficient-queue-helpers branch from 2136dfc to c1763e4 Compare March 2, 2026 14:37
@yhabteab yhabteab requested a review from julianbrost March 2, 2026 14:38
@julianbrost julianbrost merged commit 0f44133 into master Mar 3, 2026
28 checks passed
@julianbrost julianbrost deleted the icingadb-efficient-queue-helpers branch March 3, 2026 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julianbrost julianbrost julianbrost approved these changes

Assignees

No one assigned

Labels

cla/signed

Projects

None yet

Milestone

2.16.0

Development

Successfully merging this pull request may close these issues.

2 participants

@yhabteab @julianbrost