Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add new abstraction for simple Lambda that does not need explicit initialization #259

Closed
wants to merge 1 commit into from

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Apr 15, 2022

motivation: make simple things simple

changes:

  • add new abstraction names SimpleLambdaHandler that has a default initializer
  • add test

…ialization

motivation: make simple things simple

changes:
* add new abstraction names SimpleLambdaHandler that has a default initializer
* add test
@tomerd tomerd requested a review from fabianfett April 15, 2022 18:32
@tomerd tomerd changed the title add new abstraction for simple Lambdas that do not need explicit initialization add new abstraction for simple Lambda that does not need explicit initialization Apr 15, 2022
@tomerd tomerd added this to the 1.0.0-alpha.1 milestone Apr 15, 2022
@tomerd
Copy link
Contributor Author

tomerd commented Apr 15, 2022

we could / should consider doing this on LambdaHander instead of the new abstraction, also not in love with the "Simple" prefix.

@stevapple
Copy link
Contributor

We can drop context: from LambdaHandler’s initializer, then the SimpleLambdaHandler model will just fit (we can keep async and throws so users can still initialize some resources if they want).

@tomerd
Copy link
Contributor Author

tomerd commented Apr 15, 2022

@stevapple context is needed when users need to initialize resources, it provides thinks like NIO's EventLoop and Terminator, so we cannot just drop it.

@tomerd tomerd added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Apr 15, 2022
@stevapple
Copy link
Contributor

Is there any reason they cannot use async/await for asynchronous startup, and use throw in place of manual termination? I think that's the "simple" way and should work for most people.

@tomerd
Copy link
Contributor Author

tomerd commented Apr 15, 2022

termination is for resource cleanup when shutting down, there needs to be an entry point to that. also all NIO based clients (eg db client based on NIO) need access the the EventLoop for initialization to avoid thread hops

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I like the idea of making Lambda easier to use, I wonder if we actually achieve this goal through this change. Until now, IMO, it was clear which Handler protocol we recommend: LambdaHandler. Now the story is becoming harder.

Let's look at the pros and cons of this change:

Pro:

  • We can remove 1-3 lines for very simple LambdaHandlers.
  • Users don't need to specify an explicit init

Con:

  • We need to explain to users the difference between SimpleLambdaHandler and LambdaHandler even in the Simple case. We need explain to users, that they shall use SimpleLambdaHandler if they don't have any resources that they want to share between invocations.
  • The way from a very simple lambda to an actual lambda (I think every real lambda, will use some shared network resources) is becoming harder. We need to remind developers, that they need to change the Handler protocol and now need to implement a new init method. There is no compiler warning to point them into the right direction.

In my opinion the cons outweigh the pros. Is there something that I have overseen?

@tomerd
Copy link
Contributor Author

tomerd commented Oct 8, 2022

superseded by #273

@tomerd tomerd closed this Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants