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 defresolver async? options #90

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

jungwookim
Copy link
Contributor

@jungwookim jungwookim commented Jan 6, 2023

https://lacinia.readthedocs.io/en/latest/resolve/async.html 의 안내에 따랐습니다.

Caveats

Thread Pools

By default, calls to deliver! invoke the callback (provided to on-deliver!) in the same thread. This is not always desirable; for example, when using Clojure core.async, this can result in considerable processing occuring within a thread from the dispatch thread pool (the one used for go blocks). There are typically only eight threads in that pool, so a callback that does a lot of processing (or blocks due to I/O operations) can result in a damaging impact on overall server throughput.

To address this, an optional executor can be provided, via the dynamic com.walmartlabs.lacinia.resolve/callback-executor var. When a ResolverResultPromise is delivered, the executor (if non-nil) will be used to execute the callback; Java thread pools implement this interface.

@jungwookim jungwookim marked this pull request as ready for review January 9, 2023 01:20
@jungwookim jungwookim marked this pull request as draft January 9, 2023 04:04
@jungwookim jungwookim marked this pull request as ready for review January 10, 2023 02:42
[async? return-camel-case? body]
(if async?
`(let [result# (resolve/resolve-promise)]
(.start (Thread.
Copy link

@devgrapher devgrapher Jan 10, 2023

Choose a reason for hiding this comment

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

혹시 이게 모든 resolve마다 새로운 스레드를 만드는거라면 스레드가 너무 많아지는건 아닐까 싶은데..
future는 어떨까요? future는 cachedThreadPool을 쓰는것 같네요.
https://stackoverflow.com/questions/35641757/does-future-always-create-a-new-thread

혹은 커스텀하게 풀을 만들어서 사용해도 좋을거같아요!

Choose a reason for hiding this comment

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

궁극적으로는 core.async를 써야할거같지만 문서에도 나온것처럼 이건 테스트가 좀 필요한듯하여..

Copy link
Member

Choose a reason for hiding this comment

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

제가 궁금했던 부분도 이거였어요. 스레드를 비동기 리졸버에서 매번 새로 생성하면 문제가 될 것 같아서요.
저는 core.async 보다 프로미스가 좋을 것 같습니다.

Choose a reason for hiding this comment

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

간단히 테스트해보니.. future나 thread나 비슷한 결과를 보여주네요.
게시글 하나당 여러번의 필드리졸버가 호출되면서 많은 수의 스레드가 만들어지는군요.
thread를 쓴다하더라도 어차피 한번 사용하고 버려질꺼고..
future를 쓴다고 해도 동시에 여러 필드리졸버가 호출되면 이것도 어쩔수없이 스레드가 많이 생성되네요. (스레드가 재활용이된건지모르겠네요)
물론 시간이 지나면 스레드 개수는 다시 내려왔습니다.

아래 커맨드로 병렬로 10개의 요청을 보내는 작업얼 여러번 진행했습니다.
seq 1 10 | xargs -n1 -P10 curl 'http://localhost:8080/graphql' ...

스크린샷 2023-01-11 오후 2 11 54

스크린샷 2023-01-11 오후 2 21 16

스크린샷 2023-01-11 오후 1 45 20

Copy link

@devgrapher devgrapher Jan 11, 2023

Choose a reason for hiding this comment

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

그냥 superlifter가 편할까.. 싶은생각도.. 이경우 중복된 id에 대한 요청은 알아서 한번만 보내주는 효과도 있으니..
아 이건 urania로도 커버가 되긴하네요

Choose a reason for hiding this comment

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

일단은 superlifter나 promesa를 탐구해봐야겠습니다

Copy link
Contributor

@ruseel ruseel Jan 12, 2023

Choose a reason for hiding this comment

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

우리 경우엔 스레드로 보내는 대부분의 작업들이 i/o대기용인데.. 이 대기를 위해서 개별 스레드를 점유하고 있는거니까요.
그래서 뭔가 이벤트기반의 솔루션을 탐색하게 되네요..

이 댓글이 참 맞는 말이다 싶어서
검색을 한 참 하다

여기로 갔는데요.
https://martintrojer.github.io/clojure/2013/07/07/coreasync-and-blocking-io

결국 i/o 대기를 위해서 개별 쓰레드를 점유하지 않으려면

aws/invoke 를 aws/invoke-async 로 바꿔야 할 것 같아요.

Choose a reason for hiding this comment

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

오 맞아요 요것도 있었죠!

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 invoke-async를 함 츄라이 해볼래요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@devgrapher promesa 코드보니 vthread도 있네요. 코드는 보다가 시간이 없어서 여기까지만..

@devgrapher
Copy link

단순히 thread대신 promesa/future로만 바꿨는데 문제가 사라졌습니다. 스레드 개수가 100개 초반대로 유지됩니다.
응답속도도 thread쓸때와 같이 빨라졌구요.

스크린샷 2023-01-12 오전 10 21 48

ddb 요청들이 병렬로 잘 실행된 모습입니다. likeCount, author, recentComment등
스크린샷 2023-01-12 오전 10 24 26

아마도 promesa는 default로 forkJoinPool을 쓰는거 같은데 이게 잘 작동하나봅니다.

@devgrapher
Copy link

요기 브랜치에 커밋해놓고 테스트좀 해볼게요

[async? return-camel-case? body]
(if async?
`(let [result# (resolve/resolve-promise)]
(prom/future
Copy link
Contributor Author

Choose a reason for hiding this comment

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

require에 추가가 안되어있는 것 같은데, 잘 동작하나여?

Choose a reason for hiding this comment

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

이미 있었습니다. superlifter관련로직때문이었던거같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

엇 제 의미는 이 네임스페이스에서 alias가 없는데도 prom이 동작하는가? 가 궁금했는데 그게 동작을 하나보네요?! 😮

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

링크가 안가지는데 요렇게 require에 있습니다.
[promesa.core :as prom]

Choose a reason for hiding this comment

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

[promesa.core :as prom]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아앗...

@devgrapher
Copy link

devgrapher commented Jan 15, 2023

async?에 적용된 thread pool을 조금 테스트해봤습니다.
https://www.notion.so/greenlabs/async-thread-pool-6a406e12d3214ce09f1b5922710561b3

스레드 풀을 명시적으로 만들어 쓰면 좋을듯해요

This was referenced Jan 19, 2023
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.

8 participants