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.
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
refactor: Migrate S3 Client from AWS SDK v2 to v3 #220
refactor: Migrate S3 Client from AWS SDK v2 to v3 #220
Changes from 12 commits
98cf010
6f49645
889ca3e
a9a0d46
ed0fd9e
dfe0fb2
06f8483
771b533
f493800
e5dcf37
0897b90
3ff1c77
92d4c10
bf36a95
da2c3e1
2eef4a1
05b2035
0d3e766
24b9a33
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
100ms seems quite long, what is this value based on?
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.
The 100ms value was an estimate to prevent CPU overload from a tight loop, as there's no specific AWS documentation on the exact timing needed for this function. Reducing it to 10ms might be possible. Additionally, I suggest opening an issue on parse-server to make the getFileLocation function async, eliminating the need for the deasync library altogether.
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.
I reduced it to 10ms
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.
If we estimate the response time for an intra-region call from EC2 to S3 (i.e. Parse Server also running on AWS) to be ~10-100ms, then a 100ms sleep would on avg. double the response time. Maybe setting to 50ms could be a good compromise.
We can only guess a value here, because the best value depends on the actual response times of the specific infrastructure scenario. Using
deasync
is really more of a hack and we don't know how this responds under heavy concurrent load given thatdeasync.sleep
blocks the event loop. Maybe the correct approach here would be to modify Parse Server first to support sync and async calling ofgetFileLocation
, and then modify the S3 adapter. Would be open to submit a PR to Parse Server? We could add another bounty for that.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.
Thank you for the feedback! I don't currently have a Parse Server test setup, but I can certainly set one up. It might take some time, but I'm definitely up for the task. I'm happy to work on this without needing the bounty.
Could you please create an issue on the Parse Server repository for this?
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.
Done, see parse-community/parse-server#9268.
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.
Thank you! 🙏 I'll get started on it. 🚀