-
Notifications
You must be signed in to change notification settings - Fork 8
Add ValkeyClient.subscribe functions that uses one connection and.reconnects if it is lost #131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Pull request no significant performance differences ✅ SummaryNew baseline 'pull_request' is WITHIN the 'main' baseline thresholds. Full Benchmark ComparisonComparing results between 'main' and 'pull_request'
ValkeyBenchmarksClient: GET benchmark metricsMalloc (total): results within specified thresholds, fold down for details.
Client: GET benchmark | parallel 20 | 20 concurrent connections metricsMalloc (total): results within specified thresholds, fold down for details.
Connection: GET benchmark metricsMalloc (total): results within specified thresholds, fold down for details.
Connection: Pipeline benchmark metricsMalloc (total): results within specified thresholds, fold down for details.
HashSlot – {user}.whatever metricsMalloc (total): results within specified thresholds, fold down for details.
ValkeyCommandEncoder – Command with 7 words metricsMalloc (total): results within specified thresholds, fold down for details.
ValkeyCommandEncoder – Simple GET metricsMalloc (total): results within specified thresholds, fold down for details.
ValkeyCommandEncoder – Simple MGET 15 keys metricsMalloc (total): results within specified thresholds, fold down for details.
|
Was thinking I could probably extend this to the cluster client as well |
bfec042
to
30a0160
Compare
@@ -328,15 +328,13 @@ public final actor ValkeyConnection: ValkeyConnectionProtocol, Sendable { | |||
/// create a BSD sockets based bootstrap | |||
private static func createSocketsBootstrap(eventLoopGroup: EventLoopGroup) -> ClientBootstrap { | |||
ClientBootstrap(group: eventLoopGroup) | |||
.channelOption(ChannelOptions.allowRemoteHalfClosure, value: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client doesn't need half closure, and it meant we were leaving connections open if the database was closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 nits and 1 question
What's the question |
process: (AsyncMapSequence<ValkeySubscription, ValkeyKey>) async throws -> sending Value | ||
) async throws -> sending Value { | ||
try await self.subscribe(to: [ValkeySubscriptions.invalidateChannel]) { subscription in | ||
try await self.subscribe(to: [ValkeySubscriptions.invalidateChannel], isolation: isolation) { subscription in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a follow up: can we use CollectionOfOne here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this then requires some Collection
through a whole series of functions. Can we do this in a separate PR
let value: Value | ||
do { | ||
value = try await process(stream) | ||
try Task.checkCancellation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it becomes borderline impossible to exit this closure without CancellationError, is this the behavior we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not totally sure what you mean here. Its up to the contents of the closure how they exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the point with this check it isn't anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw. we must make sure that the unsubscribe job runs outside of this task, as this will always close the connection otherwise. I think we'll need to do a
await Task {
// cancellation shield
self.unsubscribe()
}.get()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkCancellation was there to ensure a CancellationError was thrown if the Task was cancelled. AsyncStream doesn't throw an error when it is cancelled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the unsubscribe into its own unstructured Task
b75dccf
to
39d99dc
Compare
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
2abf833
to
010cada
Compare
Signed-off-by: Adam Fowler <[email protected]>
Updated to only use one connection for subscriptions |
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
Signed-off-by: Adam Fowler <[email protected]>
No description provided.