-
Notifications
You must be signed in to change notification settings - Fork 0
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
jungwookim
wants to merge
9
commits into
main
Choose a base branch
from
defresolver-async-option
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
d7eded5
base async?
jungwookim 0f2f60a
fix test
jungwookim 558c89f
error response
jungwookim dd6a235
wrap-async-body 안에서도 csk 변환 (#91)
neverlish 2aa26d2
add test
jungwookim bf6eadc
promesa.core/future in async
devgrapher 02f955d
Merge branch 'main' of github.com:green-labs/gosura into defresolver-…
jungwookim 2ce5b19
Merge branch 'defresolver-async-option' of github.com:green-labs/gosu…
jungwookim 055edfa
Merge remote-tracking branch 'origin/main' into defresolver-async-option
wickedev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
혹은 커스텀하게 풀을 만들어서 사용해도 좋을거같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
궁극적으로는 core.async를 써야할거같지만 문서에도 나온것처럼 이건 테스트가 좀 필요한듯하여..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 궁금했던 부분도 이거였어요. 스레드를 비동기 리졸버에서 매번 새로 생성하면 문제가 될 것 같아서요.
저는 core.async 보다 프로미스가 좋을 것 같습니다.
There was a problem hiding this comment.
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' ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그냥 superlifter가 편할까.. 싶은생각도.. 이경우 중복된 id에 대한 요청은 알아서 한번만 보내주는 효과도 있으니..
아 이건 urania로도 커버가 되긴하네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단은 superlifter나 promesa를 탐구해봐야겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 댓글이 참 맞는 말이다 싶어서
검색을 한 참 하다
여기로 갔는데요.
https://martintrojer.github.io/clojure/2013/07/07/coreasync-and-blocking-io
결국 i/o 대기를 위해서 개별 쓰레드를 점유하지 않으려면
aws/invoke 를 aws/invoke-async 로 바꿔야 할 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 맞아요 요것도 있었죠!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 invoke-async를 함 츄라이 해볼래요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devgrapher promesa 코드보니 vthread도 있네요. 코드는 보다가 시간이 없어서 여기까지만..