-
Notifications
You must be signed in to change notification settings - Fork 13
Generic node description #230
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
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: GET benchmark – NoOpTracer metricsMalloc (total): results within specified thresholds, fold down for details.
Connection: Pipeline array 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.
|
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.
Added comments around return types.
/// - Parameter nodeDescription: Description of the node to connect to. | ||
/// - Returns: A configured `ValkeyNode` instance ready to connect to the specified node. | ||
@usableFromInline | ||
package func makeConnectionPool(nodeDescription: ValkeyNodeDescription) -> ValkeyNodeClient { |
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.
Do you intend to return ConnectionPool
here ?
package typealias ConnectionPool = ValkeyNodeClient
is defined, but ConnectionPool
is not being used in the file.
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 type conforms to the protocol ValkeyNodeConnectionPoolFactory
. This protocol has an associatedtype ConnectionPool
. When you create a concrete type that conforms to a protocol with an asssociatedtype you need to indicate what the associated type is. In this case ConnectionPool
is ValkeyNodeClient
.
You can do this by defining the protocol requirements using that type or add a typealias. We have ended up doing both here. There is no harm in this but yeah I guess it does look a little weird.
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 removed the typealias because it isn't necessary
/// - Returns: A configured `ValkeyNode` instance ready to connect to the specified node. | ||
@usableFromInline | ||
package func makeConnectionPool(serverAddress: ValkeyServerAddress) -> ValkeyNodeClient { | ||
package func makeConnectionPool(nodeDescription: ValkeyServerAddress) -> ValkeyNodeClient { |
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.
Do you intend to return ConnectionPool
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.
See above
This allows us have separate node descriptions for different implementations of ValkeyRunningClientsStateMachine Signed-off-by: Adam Fowler <adamfowler71@gmail.com>
Signed-off-by: Adam Fowler <adamfowler71@gmail.com>
Signed-off-by: Adam Fowler <adamfowler71@gmail.com>
169f245
to
3a59e06
Compare
Add associatedtype
NodeDescription
toValkeyNodeConnectionPool
.Add update
ValkeyRunningClientsStateMachine
to useNodeDescription
Add cluster specific
ValkeyClusterNodeClientFactory
which usesValkeyNodeDescription
as a node descriptionThis change is so we can use
ValkeyRunningClientsStateMachine
inValkeyClient
which requires a different node address type.