[Feedback] RedisNIO: A NIO-Based Redis Driver

RedisNIO: A NIO-based Redis Driver

The review of SSWG-0004: RedisNIO Driver starts now and runs through June 26, 2019.

This package has the git tag 1.0.0-alpha.1 for the purposes of providing a quick way to pull the implementation into projects for evaluation.

Package Name swift-redis-nio-client
Module Name RedisNIO
Proposed Maturity Level Incubating
License Apache 2
Dependencies SwiftNIO 2.x, SwiftLog 1.x, SwiftMetrics 1.x
Project Repo https://gitlab.com/mordil/swift-redis-nio-client (GitHub Mirror)

What goes into a review of a proposal?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the evolution of the server-side Swift ecosystem.

When reviewing a proposal, here are some questions to consider:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough?
  • Does this proposal fit well with the feel and direction of Swift on Server?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Thank you for contributing to the Swift Server Work Group!

What happens if the proposal gets accepted?

If this proposal gets accepted, the repository will then become usable as a SwiftPM package and a version (likely 1.0.0) will be tagged. The development (in form of pull requests) will continue as a regular open-source project.

3 Likes

This looks great! Huge +1 to this proposal and the desired sandbox maturity level.

I have some comments on the proposal below. It's mostly just small things that I think could make the API more consistent w/ Swift and NIO. But, as it stands, I think this proposal is good to move forward regardless of whether these changes are made.

Thanks, @Mordil !


RESPTranslator as a struct

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.

RESPTranslator.parseBytes using position

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.

Because of this, I think we should expect that the buffer supplied to RESPTranslator.parseBytes has the message as the first readable byte, so that we can omit the fromIndex: inout Int. This would simplify the API by relying on the great work the NIO team has already done in ByteBuffer.

RESPValueConvertible API

I think this package could benefit from a convertible-protocol pattern that Vapor's DB modules are using:

public protocol FooConvertible {
    init?(foo: Foo)
    var foo: Foo? { get }
}

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.

Bootstrap helpers as extensions on NIO types

NIO at least offers bootstrap helpers (like addHTTPHandlers()) as methods on NIO types like ChannelPipeline. Perhaps this package should consider matching that instead of using static methods on the Redis type.

sub-class vs. protocol

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.

Redis.makeConnection

I think this method would make more sense on RedisConnection itself. If I were attempting to create a new RedisConnection in my code (without reading the docs first), I would probably write something like:

import RedisNIO

let conn = RedisConnection.

After writing this, I'd inspect the auto-completion options available. At the current state, I'd see the init(channel:logger:) method which I might think to use, but not the actual method I need.

If the makeConnection (maybe call it connect?) method were statically available on RedisConnection, I would find it immediately.

Potentially init(channel:logger:) could consider being made internal, too. There's really no reason to use it outside of testing, where one could use @testable import anyway.

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

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.

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?

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

.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(...)

1 - That is fair, and I've re-read and understood API Design Guidelines (man, they're tough to get right all the time :smile:) - 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.

In this situation, create a temporary copy of the buffer first and make read calls on that. If the parsing succeeds (not the .needMoreData case), then you can set the inout buffer to the copy via buffer = copy.

Calling read... methods will never make a copy of the underlying storage. It's only a very small struct containing the offsets that is ever copied.

In other words, the custom offset being passed to RESPTranslator is doing exactly what ByteBuffer's current read index should be doing.

See an example here: https://github.com/vapor/postgres-nio/blob/master/Sources/PostgresNIO/Message/PostgresMessageDecoder.swift#L20-L50

I still think the distinction between RESPValue.null and RESPValue?.none would be useful in the future, especially considering the convert method is not throwing. But I'll defer to you here :+1:

... I don't know why I didn't think of that. :man_facepalming:

I'll add this to the list.

I'm willing to listen others to see if the majority opinion differs from mine. If I'm in the minority of seeing a distinction without a purpose I'm willing to change it to match the majority's expectation.