-
Notifications
You must be signed in to change notification settings - Fork 30
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
Fix open S3Object InputStream #71
base: master
Are you sure you want to change the base?
Fix open S3Object InputStream #71
Conversation
…make it reusable and prevent a wrong usage because of an unused open InputStream. Signed-off-by: Michael Herdt <[email protected]>
Signed-off-by: Michael Herdt <[email protected]>
Signed-off-by: Michael Herdt <[email protected]>
Signed-off-by: Michael Herdt <[email protected]>
Signed-off-by: Michael Herdt <[email protected]>
…on before closing it. Signed-off-by: Michael Herdt <[email protected]>
Signed-off-by: Michael Herdt <[email protected]>
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.
Looks good, thanks @herdt-michael 👍
@hawkbit-bot verify please |
|
||
private static DbArtifactHash createArtifactHash(final String artifactId, ObjectMetadata metadata) { | ||
return new DbArtifactHash(artifactId, BaseEncoding.base16().lowerCase() | ||
.encode(BaseEncoding.base64().decode(sanitizeEtag(metadata.getETag()))), null); |
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.
} | ||
|
||
private static S3Object getS3ObjectOrThrowException(AmazonS3 amazonS3, String bucketName, String key) | ||
throws S3ArtifactNotFoundException { |
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.
* in case that no artifact could be found for the given values | ||
*/ | ||
public static S3Artifact get(final AmazonS3 amazonS3, final S3RepositoryProperties s3Properties, final String key, | ||
final String artifactId) throws S3ArtifactNotFoundException { |
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.
Signed-off-by: Michael Herdt <[email protected]>
|
||
private void refreshS3ObjectIfNeeded() { | ||
if (s3Object == null || s3InputStream == null) { | ||
LOG.info("Initialize S3Object in bucket {} with key {}", s3Properties.getBucketName(), key); |
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 guess we could change the log level to debug otherwise we would get a lot of log messages for each file download
private void refreshS3ObjectIfNeeded() { | ||
if (s3Object == null || s3InputStream == null) { | ||
LOG.info("Initialize S3Object in bucket {} with key {}", s3Properties.getBucketName(), key); | ||
s3Object = amazonS3.getObject(s3Properties.getBucketName(), key); |
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.
why isn't the getS3ObjectOrThrowException
method used here? s3Object can be potentially null
try { | ||
return S3Artifact.get(amazonS3, s3Properties, key, sha1Hash); | ||
} catch (final S3ArtifactNotFoundException e) { | ||
LOG.debug("Cannot find artifact for bucket {} with key {}", e.getBucket(), e.getKey(), e); |
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.
here, however I would use level "warn" because we always execute existsByTenantAndSha1
beforehand in usage scenarios
} | ||
|
||
private static S3Object getS3ObjectOrThrowException(AmazonS3 amazonS3, String bucketName, String key) { | ||
final S3Object s3Object = amazonS3.getObject(bucketName, key); |
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.
we should also catch SdkClientException
as well as AmazonServiceException
here
Frankly speaking I believe that proposed changes make the code less understandable with S3Artifact being mutable and used in different context (e.g. |
Open S3Object InputStream when creating an instance of S3Artifact to make it reusable and prevent a wrong usage because of an unused open InputStream.
Signed-off-by: Michael Herdt [email protected]