-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat: Initial Classes and Interface for Transfer Manager #1874
Conversation
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.
A few minor things to change, and a question or two.
google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadJob.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadJob.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadJob.java
Outdated
Show resolved
Hide resolved
public DownloadResult build() { | ||
checkNotNull(input); | ||
checkNotNull(status); | ||
return new DownloadResult(input, outputDestination, status, exception); |
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.
TODO: validate either outputDestination
or exception
are non-null relative to the value of status
.
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 will add this in a follow up. I know its only a few lines but was trying to get through the other comments first.
google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadJob.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadResult.java
Outdated
Show resolved
Hide resolved
google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadResult.java
Outdated
Show resolved
Hide resolved
...oud-storage/src/main/java/com/google/cloud/storage/transfermanager/ParallelUploadConfig.java
Outdated
Show resolved
Hide resolved
|
||
class TransferManagerConfig { | ||
@NonNull private final int maxWorkers; | ||
@NonNull private final int perWorkerBufferSize; |
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.
Thoughts on changing this to maxTotalBufferSize
which could leave us wiggle room on how much per worker we use if we're able to determine scaling up buffer size can increase throughput.
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 feel like we are moving further from the original design which was chunkSize if we continue in this direction. I don't think this is necessarily a bad thing but node and python are also out already with chunkSize.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.