[Feedback] RedisNIO: A NIO-Based Redis Driver

[Mordil (Nathan Harris)]

For all items agreed to update, I've created swift-redis-nio-client !47 for tracking.

@tanner0101

I'd personally rather see RedisCommandHandler being a protocol w/ default extensions than an open class. IMO inheritance is only really appropriate for Objective-C compatible development (like w/ UIKit, AppKit, etc). With pure Swift, protocols + extensions can do the same job, while also being composable.

I was going to refute this as a direct contradiction made in the initial pitch thread - but after re-reading the feedback I'm reinterpreting what was said and struggling to find out why I went the route of open for the means of extension that Johannes was referring to. I'll definitely change this as I was not too keen personally on it being open either.

@tanner0101

We may want to add some configuration options for RESP translation in the future. If RESPTranslator were a struct with instance methods, we could pass those options in during init without needing to change the parsing / serialization API. This would give us more flexibility in the future.

This is true, but I don't see it being a huge nuisance to change from an enum to a struct in order to do that later (aside from it being major vs minor SemVer bump).

If it was changed, how would we feel about trying to mimic the Codable API of encode(_:) -> RESPValue and decode<Int | String | Buffer | etc>(from: RESPValue) -> T?

@tanner0101

Changing the current reader index of a ByteBuffer is a very cheap operation. Generally, you should always use the read... prefixed methods that mutate a buffer instead of the get methods plus a custom offset.

This is probably a question for the NIO team...

What happens if we move the reader index during say, a decoding pass of an array (so it'll be recursive) but then return .needMoreData in ByteToMessageDecoder? Do we get a "reset" bytebuffer each time, or will I need to now build state for the decoder?

My understanding is no, we won't.

My intention for RESPTranslator was for it to be more of an "observer" of the ByteBuffer for just parsing RESPValues out of

@tanner0101

Redis.makeConnection

I think this method would make more sense on RedisConnection itself.

@tanner0101

Bootstrap helpers as extensions on NIO types

.I originally had these, but then moved to the Redis namespacing enum to provide a pattern for higher level libraries to provide extensions on a common "type" - but I just realized this reduces the ability for Vapor to provide a top-level module named Redis.

I've added this to my list to again break these out to existing constructs.

• RedisConnection.connect(to:...)
• ClientBootstrap.makeDefaultForRedis(...)

@tanner0101

There are a few key differences with this pattern compared to RESPValueConvertible :

1: The init method uses a label. Given that very heavily used types like String and Int will conform, we should probably avoid overloading method names like init(_:) too much.
2: The convert-to method allows returning nil if the conversion fails (just like the init method does)
3. This more closely matches patterns already found in Swift like CustomStringConvertible .

1 - That is fair, and I've re-read and understood API Design Guidelines (man, they're tough to get right all the time ) - I'll update to include a label

2 & 3 - RESPValue is different from PostgresDataType and PostgresData in that the backing values are one prime primitive (String / Integer) for their underlying type - so they have a solid conversion between the two that one would expect to match the pattern of ExpressibleBy*Literal protocols. Given that RESPValue has a representation of what nil would look like with null, it's reasonable to expect conformers to just return nil if they can't express their value within a RESPValue.

While PostgresDataType does have a concept of "null" - it still has a value that can be represented elsewhere with the storage of the Int value but RESPValue's "null" is just that - it's a single representation of no value at all.