Skip to content

Add ValkeyNodeDescription, Rename ServerAddress, and Add Connection Pool Components Summary #85

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

Merged
merged 3 commits into from
Jun 2, 2025

Conversation

fabianfett
Copy link
Collaborator

This PR introduces several improvements to the node management and connection pool functionality:

  • Added ValkeyNodeDescription to represent node metadata
  • Renamed ServerAddress to ValkeyServerAddress and moved it to a dedicated file
  • Added ValkeyNodeConnectionPool protocol for managing node connections
  • Created ValkeyNodeConnectionPoolFactory protocol for connection pool instantiation

Motivation

These changes are made to allow easier testability of the cluster types that will follow in follow up PRs.

Changes

  1. ValkeyNodeDescription
    Added a new type that represents metadata about a Valkey node, containing information such as the node's address, health status, and other relevant properties.

  2. ServerAddress Rename
    Renamed ServerAddress to ValkeyServerAddress and moved it to its own file for better organization and to align with the naming convention of other components.

  3. Connection Pool Components
    Added two new protocols:

  • ValkeyNodeConnectionPool: Defines the interface for managing connections to a Valkey node
  • ValkeyNodeConnectionPoolFactory: Provides a mechanism for creating connection pools as needed

@fabianfett fabianfett requested a review from adam-fowler May 30, 2025 13:23
Copy link

github-actions bot commented May 30, 2025

✅ Pull request no significant performance differences ✅

Summary

New baseline 'pull_request' is WITHIN the 'main' baseline thresholds.

Full Benchmark Comparison

Comparing results between 'main' and 'pull_request'

Host '38bc15fe3a12' with 2 'x86_64' processors with 7 GB memory, running:
#14~24.04.1-Ubuntu SMP Thu Apr 24 17:41:03 UTC 2025

ValkeyBenchmarks

GET benchmark metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 4 4 4 4 4 11 11 11
pull_request 4 4 4 4 4 11 11 11
Δ 0 0 0 0 0 0 0 0
Improvement % 0 0 0 0 0 0 0 0

HashSlot – {user}.whatever metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 0 0 0 0 0 0 0 21
pull_request 0 0 0 0 0 0 0 21
Δ 0 0 0 0 0 0 0 0
Improvement % 0 0 0 0 0 0 0 0

ValkeyCommandEncoder – Command with 7 words metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 0 0 0 0 0 0 0 747
pull_request 0 0 0 0 0 0 0 740
Δ 0 0 0 0 0 0 0 -7
Improvement % 0 0 0 0 0 0 0 -7

ValkeyCommandEncoder – Simple GET metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 0 0 0 0 0 0 0 1956
pull_request 0 0 0 0 0 0 0 1943
Δ 0 0 0 0 0 0 0 -13
Improvement % 0 0 0 0 0 0 0 -13

ValkeyCommandEncoder – Simple MGET 15 keys metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) * p0 p25 p50 p75 p90 p99 p100 Samples
main 0 0 0 0 0 0 0 364
pull_request 0 0 0 0 0 0 0 360
Δ 0 0 0 0 0 0 0 -4
Improvement % 0 0 0 0 0 0 0 -4

/// - Note: This type is primarily used internally by the Valkey client for node management
/// in cluster configurations.
@usableFromInline
package struct ValkeyNodeDescription: Identifiable, Hashable, Sendable {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this conform to ValkeyNodeDescriptionProtocol

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to. ValkeyNodeDescription is our real currency type. But I don't want to force adopters to give us ValkeyNodeDescription directly. Instead they can use ValkeyNodeDescriptionProtocol representations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you have two APIs? One for ValkeyNodeDescriptionProtocol and one for ValkeyNodeDescription? Or are you going to create a ValkeyNodeDescription from the supplied ValkeyNodeDescriptionProtocol.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or are you going to create a ValkeyNodeDescription from the supplied ValkeyNodeDescriptionProtocol.

Internally we only use ValkeyNodeDescription. ValkeyNodeDescriptionProtocol is its public equivalent.

}
self.runStateMachineActions(actions)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have these changes been propagated back to the PostgresNIO connection pool?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet.

import NIOCore

/// Server address to connect to
public struct ValkeyServerAddress: Sendable, Equatable {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to fix the tests as they are still referencing ServerAddress

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@fabianfett fabianfett force-pushed the ff-cluster-pool-abstraction branch from 131505c to cb9a43e Compare June 2, 2025 08:31
@fabianfett fabianfett requested a review from adam-fowler June 2, 2025 09:38
@adam-fowler adam-fowler merged commit 14d53ea into main Jun 2, 2025
3 checks passed
@adam-fowler adam-fowler deleted the ff-cluster-pool-abstraction branch June 2, 2025 13:18
adam-fowler pushed a commit that referenced this pull request Jul 11, 2025
adam-fowler pushed a commit that referenced this pull request Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants