Code Guru PR test #1
Conversation
testing
| } | ||
|
|
||
| public static AmazonS3 getS3Client() { | ||
| return AmazonS3ClientBuilder.standard().withRegion(Regions.DEFAULT_REGION).build(); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer
This code is written so that the client cannot be reused across invocations of the Lambda function.
To improve the performance of the Lambda function, consider using static initialization/constructor, global/static variables and singletons. It allows to keep alive and reuse HTTP connections that were established during a previous invocation.
Learn more about best practices for working with AWS Lambda functions.
| final AmazonS3 s3Client = EventHandler.getS3Client(); | ||
| logger.log("Processing Bucket: " + bucketName); | ||
|
|
||
| ObjectListing files = s3Client.listObjects(bucketName); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer
This code might not produce accurate results if the operation returns paginated results instead of all results. Consider adding another call to check for additional results.
| final AmazonS3 s3Client = EventHandler.getS3Client(); | ||
| logger.log("Processing Bucket: " + bucketName); | ||
|
|
||
| ObjectListing files = s3Client.listObjects(bucketName); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer
This code uses an outdated API. ListObjectsV2 is the revised List Objects API, and we recommend you use this revised API for new application developments.
|
|
||
| long expirationTime = System.currentTimeMillis() + Duration.ofMinutes(1).toMillis(); | ||
| while(System.currentTimeMillis() < expirationTime) { | ||
| if (s3Client.doesObjectExist(Constants.SUMMARY_BUCKET, summaryUpdateName)) { |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer
This code appears to be waiting for a resource before it runs. You could use the waiters feature to help improve efficiency. Consider using ObjectExists or ObjectNotExists. For more information, see https://aws.amazon.com/blogs/developer/waiters-in-the-aws-sdk-for-java/
|
|
||
| if (null == lastKnownStatus || lastKnownStatus.getLeft() < timeStamp) { | ||
| lastKnownStatus = new MutablePair<Long, String>(timeStamp, status); | ||
| latestStatusForTrackingNumber.put(trackingNumber, lastKnownStatus); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer
Problem
You are using a ConcurrentHashMap, but your usage of get() and put() may not be thread-safe at lines: 110, 113, 135, and 137. Two threads can perform this same check at the same time and one thread can overwrite the value written by the other thread.
Fix
Consider replacing put() with putIfAbsent() to help prevent accidental overwriting. putIfAbsent() puts the value only if the ConcurrentHashMap does not contain the key and therefore avoids overwriting the value written there by the other thread's putIfAbsent().
More info
putIfAbsent() returns null if the value did not exist and returns the value in the map if one already exists.
View an example on GitHub (external link).
| final AmazonS3 s3Client = EventHandler.getS3Client(); | ||
| logger.log("Processing Bucket: " + bucketName); | ||
|
|
||
| ObjectListing files = s3Client.listObjects(bucketName); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code uses an outdated API. ListObjectsV2 is the revised List Objects API, and we recommend you use this revised API for new application developments.
|
|
||
| long expirationTime = System.currentTimeMillis() + Duration.ofMinutes(1).toMillis(); | ||
| while(System.currentTimeMillis() < expirationTime) { | ||
| if (s3Client.doesObjectExist(Constants.SUMMARY_BUCKET, summaryUpdateName)) { |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code appears to be waiting for a resource before it runs. You could use the waiters feature to help improve efficiency. Consider using ObjectExists or ObjectNotExists. For more information, see https://aws.amazon.com/blogs/developer/waiters-in-the-aws-sdk-for-java/
| final AmazonS3 s3Client = EventHandler.getS3Client(); | ||
| logger.log("Processing Bucket: " + bucketName); | ||
|
|
||
| ObjectListing files = s3Client.listObjects(bucketName); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code might not produce accurate results if the operation returns paginated results instead of all results. Consider adding another call to check for additional results.
|
|
||
| if (null == lastKnownStatus || lastKnownStatus.getLeft() < timeStamp) { | ||
| lastKnownStatus = new MutablePair<Long, String>(timeStamp, status); | ||
| latestStatusForTrackingNumber.put(trackingNumber, lastKnownStatus); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Problem
You are using a ConcurrentHashMap, but your usage of get() and put() may not be thread-safe at lines: 111, 114, 136, and 138. Two threads can perform this same check at the same time and one thread can overwrite the value written by the other thread.
Fix
Consider replacing put() with putIfAbsent() to help prevent accidental overwriting. putIfAbsent() puts the value only if the ConcurrentHashMap does not contain the key and therefore avoids overwriting the value written there by the other thread's putIfAbsent().
More info
putIfAbsent() returns null if the value did not exist and returns the value in the map if one already exists.
View an example on GitHub (external link).
| } | ||
|
|
||
| public static AmazonS3 getS3Client() { | ||
| return AmazonS3ClientBuilder.standard().withRegion(Regions.DEFAULT_REGION).build(); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
This code is written so that the client cannot be reused across invocations of the Lambda function.
To improve the performance of the Lambda function, consider using static initialization/constructor, global/static variables and singletons. It allows to keep alive and reuse HTTP connections that were established during a previous invocation.
Learn more about best practices for working with AWS Lambda functions.
provoking CodeGuru to respond with comments by adding new code