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 proxy support to SendToS3BucketService #358

Merged
merged 34 commits into from
Sep 1, 2023
Merged

Conversation

Beckyrose200
Copy link
Contributor

@Beckyrose200 Beckyrose200 commented Aug 16, 2023

https://eaflood.atlassian.net/browse/WATER-4086

While testing the data export service on the development environment, it was identified that the upload process was unsuccessful due to the lack of support for proxy settings.
This change adds support for proxy by checking if a proxy server is configured. If a server is configured, we add these settings to a new customConfig object. We pass this object to the S3 client.

During testing on the development environment, we were waiting 6 minutes for the S3 client to error. This was due to the default set timeout being 6 minutes long. This was far too long during testing as we had to wait until we had a readable error. This change sets the timeout to be a maximum of 10 seconds now. This config is always passed to the S3 client.

https://eaflood.atlassian.net/browse/WATER-4086

During the testing of the data export service on the development environment, it was identified that the upload process was unsuccessful due to the excessive size of the files. The original approach attempted to upload the entire file in one go. To address this issue, this pull request introduces a modification to the upload process, enabling the file to be uploaded in multiple segments. This update has logic to assess the file size and determine the most suitable method for uploading. AWS restricts the application of multi-part uploads to files under 5MB (https://docs.aws.amazon.com/AmazonS3/latest/userguide/qfacts.html). Given this constraint, certain compresses files fall below the 5MB threshold and therefore cannot be uploaded through multi-part upload. This means its necessary to include both multi-part upload and single part.
@Beckyrose200 Beckyrose200 added the bug Something isn't working label Aug 16, 2023
@Beckyrose200 Beckyrose200 self-assigned this Aug 16, 2023
@Beckyrose200 Beckyrose200 marked this pull request as ready for review August 29, 2023 09:33
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

There are just some tidy-up changes needed. For example, you've been caught out by a convention the rest of us follow, but which we've not yet written up (Add convention on line length to conventions guide)

The convention is to keep our line lengths to less than 120 chars. VS Code has a handy rulers feature which can help highlight when a line is too long to help with this.

This change also made me go away and think about conventions we should either be applying or clarifying. So, I've added a couple more that I think would be handy to apply to this change now

The final change I'd like to request is in _uploadPartCommand(). We'd discussed making partSize a true constant rather than performing a calculation for each iteration. So, for example, const FIVE_MB_IN_BYTES = 5242880.

Shout if anything is unclear or you have any objections!

Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

More comments now away from our conventions.

A key thing to think about is

  • are the JSDoc params adding value where they are used
  • if their primary value is explaining that x actually means y, should we just be thinking of a better name for the x param

There are also some corrections and suggestions. Shout if you have any questions.

app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
app/services/data/export/send-to-s3-bucket.service.js Outdated Show resolved Hide resolved
@Beckyrose200 Beckyrose200 changed the title Multipart upload for AWS bucket Add proxy support to SendToS3BucketService Sep 1, 2023
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

@Beckyrose200 Beckyrose200 merged commit a7f4a37 into main Sep 1, 2023
@Beckyrose200 Beckyrose200 deleted the multipart-upload branch September 1, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants