[DNM][RFC] alternative api #18

tomerd · 2020-03-07T08:43:15Z

do not merge, for discussion (#11) purpose

fabianfett

Great progress! The typealiases are on their way out. More comments inline.

fabianfett · 2020-03-07T12:06:25Z

Sources/SwiftAwsLambda/Lambda+Codable.swift

-    func encode(_: Out) -> Result<[UInt8], Error> { fatalError("not implmented") }
-    func decode(_: [UInt8]) -> Result<In, Error> { fatalError("not implmented") }
+    func encode(_: Out) -> Result<ByteBuffer, Error> { fatalError("not implmented") }
+    func decode(_: ByteBuffer) -> Result<In, Error> { fatalError("not implmented") }


Just out of curiosity why do we use Result here and not plain old throws?

mostly my personal dislike of try/catch blocks

fabianfett · 2020-03-07T12:25:04Z

Sources/SwiftAwsLambda/Lambda.swift

-    }
+public protocol InitializableLambdaHandler {
+    /// Initializes the `LambdaHandler`.
+    func initialize(promise: EventLoopPromise<Void>)


I can totally see the idea of using the initialize(promise: EventLoopPromise<Void>) method to report errors during startup as discussed in #11 and I think the intention is very good.

I'm not sure though if it would be better to go all in and make the initialize a real init?

For example we could set it up like this

public protocol InitializableLambdaHandler { @inlinable /// Initializes the `LambdaHandler`. func init(promise: EventLoopPromise<Void>, eventLoopGroup: EventLoopGroup) throws }

I can see that this looks kind of ugly injecting the EventLoopPromise for reporting async errors and having throws for immediate errors that prevent one from setting up the Handler object/struct. But thinking about usecases I can imagine having clean mains.

Lambda.run(MyLambdaHandler.self)

struct MyLambdaHandler: InitializableLambdaHandler, LambdaHandler { let httpClient: AsyncHTTPClient init(promise: EventLoopPromise<Void>, eventLoopGroup: EventLoopGroup) throws { self.httpClient = AsyncHTTPClient(.shared(eventLoopGroup)) // other setup with eventLoopGroup: database clients, aws sdk, whatevar. // in context of aws especially important is getting keys from secret store, // which is why the promise might be important promise.succeed(Void()) } func handle() { ... } }

Getting access to the eventLoopGroup created by the Lambda could be a huge benefit, especially for creating clients.

on one hand I like the idea of init instead of a separate initialize, on the other hand this would not be a normal ctor as you would be expected to compete the promise, so would be strange API either way.

I also dont think we need to make it a throwable. if you want to return an error catch it and fail the promise

I don't know if it would compile though. ;)

struct MyLambdaHandler: InitializableLambdaHandler, LambdaHandler { let httpClient: AsyncHTTPClient init(promise: EventLoopPromise<Void>, eventLoopGroup: EventLoopGroup) { if somethingUnexpected { return // not everything is initialized here } self.httpClient = AsyncHTTPClient() }

If we want to report an error during bootup, we could also offer a static function. Then we don't have to do the init dance.

Lambda.reportBootupError(_: error)

another alternative is to define an optional blocking initializer and that's it, so users won't be able to to any significant work in it but the signature can be simplified to:

init(eventLoop: EventLoop) throws

but this closes the window on doing things like getting one-time-initialization-type-data from some database or upstream service, which could be handy

or another option is to have both: aninit(eventLoop: EventLoop) throws where you can do things like setup you http-client, and also abootstrap(eventLoop: EventLoop, promise: Promise<Void>) for the getting one-time-initialization-type-data-from-some-database-or-upstream-service use case.

both could/should be optional. maybe that's the best?

We should insert the complete EventLoopGroup. In Lambdas with more than 1500mb ram, one gets access to two cores. Developers should be allowed to use them ;)

I don't know if we can make an init(eventLoop: EventLoop) throws optional.

fabianfett · 2020-03-07T12:32:28Z

Sources/SwiftAwsLambda/LambdaRuntimeClient.swift

@@ -33,17 +33,17 @@ internal struct LambdaRuntimeClient {
    }

    /// Requests work from the Runtime Engine.
-    func requestWork(logger: Logger) -> EventLoopFuture<(LambdaContext, [UInt8])> {
+    func requestWork(logger: Logger) -> EventLoopFuture<(LambdaContext, ByteBuffer)> {


As stated in the other pr, I'd really like to get LambdaContext out of LambdaRuntimeClient by introducing an Invocation struct, that reflect the API response more closely and then creating the LambdaContext with the Invocation in the LambdaRunner. With this we would have a much cleaner separation of concerns.

A proposal can be found here: #19

fabianfett · 2020-03-07T12:33:47Z

Sources/SwiftAwsLambda/Lambda.swift

@@ -289,6 +277,8 @@ public struct LambdaContext {
    public let clientContext: String?
    public let deadline: String?
    // utliity
+    public let eventLoop: EventLoop
+    public let allocator: ByteBufferAllocator


+1 for having the ByteBufferAllocator in the context as well.

fabianfett · 2020-03-07T12:37:18Z

Sources/SwiftAwsLambda/Lambda+Codable.swift

+            guard let data = buffer.getData(at: buffer.readerIndex, length: buffer.readableBytes) else {
+                throw Errors.invalidBuffer
+            }
+            return .success(try self.decoder.decode(In.self, from: data))


You import NIOFoundationCompat that's why you should just be able to:

return .success(try self.decoder.decode(In.self, from: buffer))

iirc that returns an optional, so we would end up with same # loc

I'm afraid if something is invalid it throws, otherwise returns a nonoptional.

https://github.com/apple/swift-nio/blob/ffb196f5441b2720c847a2170677f7ba468e030b/Sources/NIOFoundationCompat/Codable%2BByteBuffer.swift#L108

fabianfett · 2020-03-07T13:00:10Z

Sources/SwiftAwsLambda/Lambda+String.swift

@@ -12,6 +12,8 @@
 //
 //===----------------------------------------------------------------------===//

+import NIO


Do we maybe want to remove this file for now? When playing around with the handler syntax, this is just a shore to fix. Could be added back later, without any problems.

fabianfett · 2020-03-07T13:04:13Z

Sources/SwiftAwsLambda/Lambda.swift

-    /// Initializes the `LambdaHandler`.
-    func initialize(callback: @escaping LambdaInitCallBack)
-    func handle(context: LambdaContext, payload: [UInt8], callback: @escaping LambdaCallback)
+    func handle(context: LambdaContext, payload: ByteBuffer, promise: EventLoopPromise<ByteBuffer>)


As stated in #11 (comment) the returning ByteBuffer should be optional.

fabianfett · 2020-03-07T16:04:07Z

Sources/SwiftAwsLambda/Lambda+Codable.swift

        switch self.codec.decode(payload) {
        case .failure(let error):
-            return callback(.failure(Errors.requestDecoding(error)))
+            return promise.fail(Errors.requestDecoding(error))
        case .success(let payloadAsCodable):
            self.handle(context: context, payload: payloadAsCodable) { result in


For every non EventLoopPromise based callback I'd build an offloaded sync/async version.

tomerd · 2020-03-08T09:51:37Z

@fabianfett created #21 with some more ideas

tomerd · 2020-03-11T20:33:48Z

replaced by #33

tomerd requested a review from fabianfett March 7, 2020 08:43

tomerd mentioned this pull request Mar 7, 2020

Discussion: Make API SwiftNIOer #11

Closed

tomerd force-pushed the api branch from cb6f4ae to fbd1f40 Compare March 7, 2020 09:00

alternative api

07c61f2

tomerd force-pushed the api branch from fbd1f40 to 07c61f2 Compare March 7, 2020 09:10

fabianfett reviewed Mar 7, 2020

View reviewed changes

tomerd closed this Mar 11, 2020

fabianfett deleted the api branch April 28, 2020 06:49

[DNM][RFC] alternative api #18

[DNM][RFC] alternative api #18

Uh oh!

Conversation

tomerd commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabianfett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Mar 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianfett Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Mar 8, 2020

Uh oh!

tomerd commented Mar 11, 2020

Uh oh!

Uh oh!

tomerd commented Mar 7, 2020 •

edited

Loading

fabianfett Mar 7, 2020 •

edited

Loading

tomerd Mar 8, 2020 •

edited

Loading

fabianfett Mar 7, 2020 •

edited

Loading

fabianfett Mar 7, 2020 •

edited

Loading