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 Armeria server interpreters #1830

Merged
merged 25 commits into from
Feb 18, 2022
Merged

Add Armeria server interpreters #1830

merged 25 commits into from
Feb 18, 2022

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Feb 4, 2022

Motivation:

Armeria is a reactive microservice framework.
This PR includes Armeria server interpreters for Standard Future, Cats Effect, and ZIO.

Modifications:

  • Add Armeria{Future,Cats,Zio}ServerInterpreter
  • Add various mapping for converting from and to Armeria types
  • Add documents and examples

Result:

Users can choose Armeria as a server-side backend for tapir.

Future work:

Implement Armeria client interceptors as well.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um
Motivation:

[Armeria](https://armeria.dev/) is a reactive microservice framework.
This PR includes Armeria server interpreters for Standard `Future`, Cats Effect, and ZIO.

Modifications:

- Add `Armeria{Future,Cats,Zio}ServerInterpreter`
- Add various mapping for converting from and to Armeria types
- Add document and examples

Result:

Users can choose Armeria as a server-side backend for tapir.

Future work:

Implement Armeria client intercepters as well.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um
doc/server/armeria.md Outdated Show resolved Hide resolved
doc/server/armeria.md Outdated Show resolved Hide resolved
@@ -40,7 +41,10 @@ class ServerRejectTests[F[_], ROUTE](
testServer(endpoint.get.in("path"), "should return 404 for an unsupported method, when a single endpoint is interpreted")((_: Unit) =>
pureResult(().asRight[Unit])
) { (backend, baseUri) =>
basicRequest.post(uri"$baseUri/path").send(backend).map(_.code shouldBe StatusCode.NotFound)
val statusCode =
if (useMethodNotAllowedForUnsupportedMethod) StatusCode.MethodNotAllowed
Copy link
Member

Choose a reason for hiding this comment

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

isn't this functionality provided by the RejectInterceptor? It would seem this should be independent of the interpreter used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An unknown HTTP method is rejected at the routing level of Armeria before a TapirService handles the request. So RejectInterceptor can not handle the result. As a workaround, I can add an HTTP decorator of Armeria to replace "405 Method Not Allowed with Not Found.

Copy link
Member

Choose a reason for hiding this comment

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

Ah as you extract the endpoint coordinates beforehand and provide them to armeria. I haven't read this code fragment yet entirely :)

Makes sense, if that's how Armeria works, let's keep it this way. Probably worth mentioning in the docs that the behavior here diverges from the other interpreters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed HTTP methods specified in an Endpoint from Route of Armeria.
All HTTP methods are allowed at Armeria level so that RejectInterceptor customizes the response of invalid requests.

@adamw
Copy link
Member

adamw commented Feb 14, 2022

A really solid PR, thanks! :) I've left some comments

ikhoon and others added 9 commits February 16, 2022 02:46

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@adamw
Copy link
Member

adamw commented Feb 16, 2022

I also noticed that the tests are quite slow. I suppose that's because for each test, a new thread pool is created. Maybe there is a way to share these resources across tests? That's what happens e.g. in the netty interpreter (https://github.com/softwaremill/tapir/blob/master/server/netty-server/src/test/scala/sttp/tapir/server/netty/NettyFutureTestServerInterpreter.scala#L35)

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um
@ikhoon
Copy link
Contributor Author

ikhoon commented Feb 17, 2022

Maybe there is a way to share these resources across tests?

Yes. Let me specify shared pools for event loops and blocking task executors.

@ikhoon
Copy link
Contributor Author

ikhoon commented Feb 17, 2022

I found out that Armeria already uses a CommonPools if no options are specified. I guess a new thread pool would not be created for each test. I'll try to find out more about the cause of the problem.

adamw and others added 4 commits February 17, 2022 11:04

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um
@ikhoon
Copy link
Contributor Author

ikhoon commented Feb 18, 2022

I also noticed that the tests are quite slow. I suppose that's because for each test, a new thread pool is created. Maybe there is a way to share these resources across tests? That's what happens e.g. in the netty interpreter (master/server/netty-server/src/test/scala/sttp/tapir/server/netty/NettyFutureTestServerInterpreter.scala#L35)

I found out Armeria server always waits for 2 seconds to let the boss group gracefully finish all remaining tasks in the queue.
So, I ignored the close future for fast iteration. Now, an Armeria module finishes all tests in 10 seconds.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ikhoon Ikhun Um
@adamw adamw merged commit 81d9f3b into softwaremill:master Feb 18, 2022
@adamw
Copy link
Member

adamw commented Feb 18, 2022

Thanks :)

@ikhoon ikhoon deleted the armeria branch February 18, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants