LambdaRuntimeClient works with Invocation #19

fabianfett · 2020-03-07T15:07:48Z

Motivation:

We want to store different entities that are needed when executing a handler within the LambdaContext (Logger, EventLoop, ByteBufferAllocator, …)
Currently the LambdaRuntimeClient creates the LambdaContext. Having the LambdaContext with the Logger, EventLoop and ByteBufferAllocator be created from the LambdaRuntimeClient feels too much for me.
Conceptionally the Lambda control plane api call is “get next Invocation” (API naming)

Changes:

LambdaRuntimeClient responds with an Invocation and does not use the LambdaContext at all anymore.
LambdaRunner creates the LambdaContext with the Invocation and Logger (easy to add EventLoop and ByteBufferAllocator – not for this PR).
LambdaContext has been renamed to Lambda.Context
Lambda.Context has become a class, since it is conceptually not a value type and might be passed around a lot
Lambda.Context properties traceId, invokedFunctionArn, deadline are not optional anymore since they will be always set when executing a lambda
Creating an Invocation can fail with LambdaRuntimeClientError.invocationMissingHeader(String), if non optional headers are not present
the test MockLambdaServer and the performance test MockServer always return headers for deadline, traceId and function arn (static for now – could be changed with Behaviour flag – new PR?). This leads to decreased performance in performance tests, but is probably closer to real life behaviour.

Open ends:

we will need to build some kind of Deadline into the context (See also Replacing Date #9 - probably for a different PR)
we have a stupid mapping between ByteBuffer and [UInt8] in the LambdaRunner for now (marked with two TODOs). I don’t want to change this in this PR since it will lead to huge merge conflicts down the road with the potentiall API changes we have in mind.

swift-server-bot · 2020-03-07T15:07:50Z

Can one of the admins verify this patch?

swift-server-bot · 2020-03-07T15:07:50Z

Can one of the admins verify this patch?

tomerd

+1 i like this change!

tomerd · 2020-03-07T17:40:40Z

@swift-server-bot add to whitelist

tomerd · 2020-03-07T17:41:30Z

@swift-server-bot test this please

tomerd · 2020-03-07T21:15:08Z

Sources/SwiftAwsLambda/LambdaRunner.swift

@@ -117,6 +118,20 @@ private extension LambdaHandler {
    }
 }

+private extension LambdaContext {
+    init(logger: Logger, eventLoop: EventLoop, invocation: Invocation) {
+


nit: remove empty line

swiftformat . has been run.

tomerd · 2020-03-07T21:16:03Z

Sources/SwiftAwsLambda/LambdaRuntimeClient.swift

@@ -151,6 +149,7 @@ internal struct JsonCodecError: Error, Equatable {
 internal struct ErrorResponse: Codable {
    var errorType: String
    var errorMessage: String
+


remove empty line (use swiftformat .)

swiftformat . has been run.

tomerd · 2020-03-07T21:16:37Z

Sources/SwiftAwsLambda/LambdaRuntimeClient.swift

+    let cognitoIdentity   : String?
+
+    init(headers: HTTPHeaders) throws {
+


swiftformat . has been run.

tomerd · 2020-03-07T21:17:39Z

Sources/SwiftAwsLambda/LambdaRuntimeClient.swift

-        if requestId.isEmpty {
-            return nil
+
+        guard let unixTimeMilliseconds = headers[AmazonHeaders.deadline].first else {


is it mandatory in AWS Lambda API, and if so should the lambda library enforce it? probably for different PR if the answer is yes

Yes, the deadline is mandatory and we should enforce it. As said in #9 we should find a good solution how we store time here. The Context should have some kind of remaining() function that returns the time till the deadline. Also creating a timeout handler that fires a second before the actual deadline is thinkable. As you stated this is something for a different PR though.

tomerd · 2020-03-07T21:17:58Z

Sources/SwiftAwsLambda/LambdaRuntimeClient.swift

}
+

empty lines

swiftformat . has been run.

tomerd · 2020-03-07T21:18:50Z

Sources/SwiftAwsLambda/LambdaRuntimeClient.swift

+        self.deadlineDate       = unixTimeMilliseconds
+        self.invokedFunctionArn = invokedFunctionArn
+        self.traceId            = traceId
+        self.clientContext      = headers["Lambda-Runtime-Client-Context"].first


not sure but maybe headers.first("Lambda-Runtime-Client-Context") is optimized?

tomerd · 2020-03-07T21:23:23Z

@swift-server-bot test this please

tomerd · 2020-03-08T00:09:09Z

@swift-server-bot ok to test

tomerd · 2020-03-08T00:12:04Z

ah this is not triggering CI because its targeting the api branch instead of master. should we take this into master instead?

tomerd · 2020-03-08T00:39:31Z

Open ends:

we will need to build some kind of Deadline into the context (See also #9 - probably for a different PR)

Invocation is currently a struct and I'd assume copied four times with six internal Strings. Maybe we can make this faster by using an internal Storage class. (potentially 4 ref counts vs 24).

Invocation should be immutable, right? so reference could work imo

I don’t know if Context really is a value type? Especially if we now add Logger, EventLoop, ByteBufferAllocator to the Context. grpc-swift uses a class for its context:
https://github.com/grpc/grpc-swift/blob/nio/Sources/GRPC/ServerCallContexts/ServerCallContext.swift

same argument as invocation?

What do you think about namespacing LambdaContext within Lambda so that we only have Context. Or when used external Lambda.Context

+1 was planning to do that

fabianfett · 2020-03-08T11:57:10Z

@tomerd This is now a PR against master. Ready for final review and merge.

tomerd · 2020-03-08T19:42:15Z

@fabianfett lets rebase

### Motivation: - We want to store different entities that are needed when executing a handler within the LambdaContext (Logger, EventLoop, ByteBufferAllocator, …) - Currently the LambdaRuntimeClient creates the LambdaContext. Having the LambdaContext with the Logger, EventLoop and ByteBufferAllocator be created from the LambdaRuntimeClient feels to me too much for me. - Conceptionally the Lambda control plane api call is “get next Invocation” (API naming) ### Changes: - LambdaRuntimeClient responds with an Invocation and does not use the LambdaContext at all anymore. - LambdaRunner creates the LambdaContext with the Invocation, Logger and EventLoop. - LambdaContext has been renamed to Lambda.Context - Lambda.Context is a class now, since it is conceptionally not a value type and might be passed around a lot - Lambda.Context properties `traceId`, `invokedFunctionArn`, `deadline` are not optional anymore since they will be always set when executing a lambda - Creating an Invocation can fail with LambdaRuntimeClientError.invocationMissingHeader(String), if non optional headers are not present - the test MockLambdaServer and the performance test MockServer always return headers for deadline, traceId and function arn (static for now – could be changed with Behaviour flag?!) ### Open ends: - we will need to build some kind of Deadline into the context (See also #9 - probably for a different PR) - we have a stupid mapping between ByteBuffer and [UInt8] in the LambdaRunner for now (marked with two TODOs). I don’t want to change this in this PR since it will lead to huge merge conflicts down the road with the potentiall API changes we have in mind.

fabianfett · 2020-03-08T20:10:15Z

@tomerd done

fabianfett requested a review from tomerd March 7, 2020 15:07

fabianfett changed the title ~~WIP: LambdaRuntimeClient works with Invocation~~ [DNM] LambdaRuntimeClient works with Invocation Mar 7, 2020

fabianfett mentioned this pull request Mar 7, 2020

[DNM][RFC] alternative api #18

Closed

tomerd reviewed Mar 7, 2020

View reviewed changes

fabianfett force-pushed the ff-api-invocation branch from 8fda4d9 to ef7f065 Compare March 7, 2020 20:14

fabianfett changed the title ~~[DNM] LambdaRuntimeClient works with Invocation~~ LambdaRuntimeClient works with Invocation Mar 7, 2020

tomerd reviewed Mar 7, 2020

View reviewed changes

fabianfett changed the base branch from api to master March 8, 2020 11:43

fabianfett force-pushed the ff-api-invocation branch 2 times, most recently from d59d850 to a7aa066 Compare March 8, 2020 11:55

fabianfett force-pushed the ff-api-invocation branch from a7aa066 to 99d5816 Compare March 8, 2020 12:04

fabianfett mentioned this pull request Mar 8, 2020

[DNM][RFC] API-2 #21

Closed

fabianfett force-pushed the ff-api-invocation branch from 99d5816 to d0ce1f3 Compare March 8, 2020 20:08

tomerd approved these changes Mar 8, 2020

View reviewed changes

tomerd merged commit ddee38c into master Mar 8, 2020

tomerd deleted the ff-api-invocation branch March 10, 2020 18:19

		let cognitoIdentity : String?

		init(headers: HTTPHeaders) throws {

LambdaRuntimeClient works with Invocation #19

LambdaRuntimeClient works with Invocation #19

Uh oh!

Conversation

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

Uh oh!

Motivation:

Changes:

Open ends:

Uh oh!

swift-server-bot commented Mar 7, 2020

Uh oh!

swift-server-bot commented Mar 7, 2020

Uh oh!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Mar 7, 2020

Uh oh!

tomerd commented Mar 7, 2020

Uh oh!

tomerd 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!

tomerd 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd commented Mar 7, 2020

Uh oh!

tomerd commented Mar 8, 2020

Uh oh!

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

Uh oh!

Uh oh!

tomerd commented Mar 8, 2020

Uh oh!

fabianfett commented Mar 8, 2020

Uh oh!

tomerd commented Mar 8, 2020

Uh oh!

fabianfett commented Mar 8, 2020

Uh oh!

Uh oh!

fabianfett commented Mar 7, 2020 •

edited

Loading

tomerd Mar 7, 2020 •

edited

Loading

tomerd Mar 7, 2020 •

edited

Loading

tomerd commented Mar 8, 2020 •

edited

Loading