-
-
Notifications
You must be signed in to change notification settings - Fork 88
refactor: Migrate S3 Client from AWS SDK v2 to v3 #220
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
Closed
Closed
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
98cf010
migrate initialization and createBucket function
vahidalizad 6f49645
install aws s3 v3 packages
vahidalizad 889ca3e
add credentials to set-up tests
vahidalizad a9a0d46
migrate endpoint test to v3 pattern
vahidalizad ed0fd9e
migrate aws functions to v3
vahidalizad dfe0fb2
fix testing of presignedUrl
vahidalizad 06f8483
fix mock s3 client for the create file tests
vahidalizad 771b533
add some tests inline to debug
vahidalizad f493800
fix output of getFileData to be a full Buffer
vahidalizad e5dcf37
add fileStream tests
vahidalizad 0897b90
change handleFileStream response handling
vahidalizad 3ff1c77
setup fake adapter for testing to work offline as well
vahidalizad 92d4c10
Update index.js
mtrezza bf36a95
remove dotenv package
vahidalizad da2c3e1
reduce the wating time on resolving the function getSignedUrl
vahidalizad 2eef4a1
remove unnecessary initialization comments
vahidalizad 05b2035
fix handleFileStream error handling
vahidalizad 0d3e766
fix test getFileStream tests to run offline
vahidalizad 24b9a33
expose location on createFile response
vahidalizad 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 hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
logs | ||
*.log | ||
npm-debug.log* | ||
.env | ||
|
||
# Runtime data | ||
pids | ||
|
This file contains hidden or 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 hidden or 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
Oops, something went wrong.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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
Uh oh!
There was an error while loading. Please reload this page.
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. 🚀