-
Notifications
You must be signed in to change notification settings - Fork 27
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
S3Repo now uses Async client and TransferManager #613
Conversation
Signed-off-by: Chris Helma <chelma+github@amazon.com>
Signed-off-by: Chris Helma <chelma+github@amazon.com>
@@ -77,4 +77,8 @@ public Path getBlobFilePath(String indexId, int shardId, String blobName) throws | |||
Path filePath = shardDirPath.resolve(blobName); | |||
return filePath; | |||
} | |||
|
|||
public void prepBlobFiles(ShardMetadata.Data shardMetadata) throws IOException { |
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.
This goes for other implementing methods here too, but curious why you aren't including @Override
. I've found it useful especially when methods have default implementations
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.
Oh - completely missed this. Yeah, probably should have done that but I'm a bit baffled why it worked without it? Seems like the compiler should have enforced this... Java noob problems, I guess.
Will add, and will learn what that annotation actually does 😛
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!
public static S3Repo create(Path s3LocalDir, S3Uri s3Uri, String s3Region) { | ||
S3AsyncClient s3Client = S3AsyncClient.crtBuilder() | ||
.credentialsProvider(DefaultCredentialsProvider.create()) | ||
.region(Region.of(s3Region)) |
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 specifying a retry configuration
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.
Good catch - will do!
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
|
||
import java.util.Comparator; | ||
import java.util.Optional; | ||
|
||
public class S3Repo implements SourceRepo { | ||
private static final Logger logger = LogManager.getLogger(S3Repo.class); | ||
private static final double S3_TARGET_THROUGHPUT_GIBPS = 10.0; // Arbitrarily chosen |
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.
Appears by default, target throughput contributes to a memory usage that could exhaust resources. We should also specify the relatively new attribute maxNativeMemoryLimitInBytes
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.
Good idea - will investigate!
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
public static S3Repo create(Path s3LocalDir, S3Uri s3Uri, String s3Region) { | ||
S3AsyncClient s3Client = S3AsyncClient.crtBuilder() | ||
.credentialsProvider(DefaultCredentialsProvider.create()) | ||
.region(Region.of(s3Region)) |
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.
should we add crossRegionAccessEnabled
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.
Good question; I think the answer here is "no" until there's a clear need for it. It will help enforce regionalization which, in general, is a good thing.
Signed-off-by: Chris Helma <chelma+github@amazon.com>
|
||
import java.util.Comparator; | ||
import java.util.Optional; | ||
|
||
public class S3Repo implements SourceRepo { | ||
private static final Logger logger = LogManager.getLogger(S3Repo.class); | ||
private static final double S3_TARGET_THROUGHPUT_GIBPS = 8.0; // Arbitrarily chosen | ||
private static final long S3_MAX_MEMORY_BYTES = 1024L * 1024 * 1024; // Arbitrarily chosen |
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.
nit: a bit hard to understand what this is (1.073 gb), i'd prefer something like
private static final long S3_MAX_MEMORY_BYTES = 1_000_000_000L; // Minimum value (1 GB)
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.
Ah - I'm pretty sure system memory uses GiB, not GB, right? In other words - it's base 2 not base 10. A quick internet search seems to confirm.
Description
Issues Resolved
Testing
Compare that to:
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.