Add ValkeyClusterDescription #34
Conversation
fabianfett
changed the title
fabianfett
force-pushed
the
ff-add-ValkeyClusterDescription
branch
from
801c7e6 to
72d52b4
Compare
|
✅ 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'ValkeyBenchmarksGET benchmark metricsMalloc (total): results within specified thresholds, fold down for details.
HashSlot – {user}.whatever metricsMalloc (total): results within specified thresholds, fold down for details.
RESPCommandEncoder – Command with 7 words metricsMalloc (total): results within specified thresholds, fold down for details.
RESPCommandEncoder – Simple GET metricsMalloc (total): results within specified thresholds, fold down for details.
RESPCommandEncoder – Simple MGET 15 keys metricsMalloc (total): results within specified thresholds, fold down for details.
|
fabianfett
force-pushed
the
ff-add-ValkeyClusterDescription
branch
from
72d52b4 to
ffbfc9e
Compare
fabianfett
changed the title
adam-fowler
left a comment
adam-fowler
left a comment
There was a problem hiding this comment.
Mostly looks ok. Cluster description is fairly complex. I would prefer if we try to use the constructs that are already in place.
| package var token: RESPToken | ||
| } | ||
|
|
||
| package struct ValkeyClusterDescription: Hashable, Sendable { |
There was a problem hiding this comment.
Can we conform this to RESPTokenRepesentable instead of rolling our own RESPToken conversion. This would allow us to set CLUSTER.SHARDS.Response to ValkeyClusterDescription. Although that requires a change in the command renderer to ensure we aren't attempting to set the Response type for that function.
There was a problem hiding this comment.
The change to the command renderer is in, so you can override the response now
extension CLUSTER.SHARDS {
public typealias Response = ValkeyClusterDescription
}| } | ||
| } | ||
|
|
||
| extension String { |
There was a problem hiding this comment.
Can we use String(from: RESPToken) and extend it if we need the numbers as strings
| } | ||
| } | ||
|
|
||
| extension Int64 { |
There was a problem hiding this comment.
Can we conform this to RESPTokenRepresentable and move this conversion to RESPTokenRepresentable.swift
| import NIOCore | ||
| import Valkey | ||
|
|
||
| enum RESP3Value: Hashable { |
There was a problem hiding this comment.
Is it worthwhile just extending RESPToken.Value instead of creating another type
EDIT: Actually ignore that you wouldn't be able to create an array.
| func testClusterDescription() throws { | ||
| let val = RESP3Value.array([ | ||
| .array([ | ||
| .simpleString(.init(string: "slots")), |
There was a problem hiding this comment.
Valkey probably uses bulkString for these, simpleString is used for things like the ok returned from functions that have no return value.
| enum RESP3Value: Hashable { | ||
| case simpleString(ByteBuffer) | ||
| case simpleError(ByteBuffer) | ||
| case bulkString(ByteBuffer) |
There was a problem hiding this comment.
I know bulkString etc can be used to represent non-UTF8 data, but do we need this for what is essentially a debug tool. It'll make using it a little easier to use if we just use String and will match the output of #50
There was a problem hiding this comment.
does that function fix your concerns?
static func bulkString(_ value: String) -> RESP3Value {
return .bulkString(ByteBuffer(string: value))
}```
fabianfett
force-pushed
the
ff-add-ValkeyClusterDescription
branch
from
9575927 to
92f0b41
Compare
adam-fowler
left a comment
adam-fowler
left a comment
There was a problem hiding this comment.
Looks good. Just some minor comments, which can be ignored if you so feel
| .push, | ||
| .set, | ||
| .null, | ||
| .map: |
There was a problem hiding this comment.
I know you want to include all possible cases explicitly here. But given that if a new type was added it would be require a new version of the protocol this seems overkill. I'm happy to let this through but don't think it should be a requirement for all the decode functions.
There was a problem hiding this comment.
yeah. really not a fan of all the defaults everywhere. There was never a time, where default hasn't bit me
| .bigNumber(let buffer), | ||
| .simpleError(let buffer), | ||
| .blobError(let buffer): | ||
| self = buffer |
There was a problem hiding this comment.
Should errors be allowed to convert to a String or ByteBuffer? They are never presented to the user as a returned value, they have already been converted to a thrown error.
There was a problem hiding this comment.
I think what you are talking about is semantics build on top of RESP, whereas a conversion in the RESP world is absolutely valid.
| while let nodeKey = nodeIterator.next(), let key = try? String(from: nodeKey), let nodeVal = nodeIterator.next() { | ||
| switch key { | ||
| case "id": | ||
| id = try? String(from: nodeVal) |
There was a problem hiding this comment.
Any reason you don't convert straight to an Int?
There was a problem hiding this comment.
id is a String?
This adds a structured response to the
CLUSTER SHARDScommand.