Allow configuration of IdleQueueRetentionPeriodSeconds on temporary queue client #42

adam-aws · 2020-06-19T18:29:43Z

Issue #38

Description of changes:
Allow configuration of IdleQueueRetentionPeriodSeconds on temporary queue client

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ueue client

VeniXuan · 2020-06-23T00:32:38Z

Glad to see this code check in. We are using this library for our non production set up and saw high amounts of the sqs throttling exception. Wish to use this commit soon! Thanks!

robin-aws

Thanks for taking this on! A couple of things to fix, and I still want to block all PRs until we address #41.

robin-aws · 2020-06-23T19:21:57Z

src/test/java/com/amazonaws/services/sqs/AmazonSQSTemporaryQueuesClientIT.java

+ .withQueueRetentionPeriodSeconds("500");
+ client = AmazonSQSTemporaryQueuesClient.make(requesterBuilder);
+
+ Assert.fail("Shouldn't be able to create a temporary queue with QueueRetentionPeriodSeconds not between 1 and 300 seconds");


Can we upgrade to JUnit 5 and use assertThrows instead?

Added assertThrows, will create a separate issue to upgrade all tests to junit5.

robin-aws · 2020-06-23T19:23:08Z

src/main/java/com/amazonaws/services/sqs/AmazonSQSRequesterClientBuilder.java

@@ -16,6 +16,7 @@

 private int idleQueueSweepingPeriod = 5;
 private TimeUnit idleQueueSweepingTimeUnit = TimeUnit.MINUTES;
+ private String queueRetentionPeriodSeconds = AmazonSQSTemporaryQueuesClientBuilder.QUEUE_RETENTION_PERIOD_SECONDS_DEFAULT;


This should definitely be a Long everywhere instead. It was only a String before because it was hard-coded and only every used as a queue attribute, but now we should only be converting to and from strings in the code that reads and writes that queue attribute.

Made the change to long

adam-aws · 2020-06-24T19:02:34Z

AWS CodeBuild CI Report

CodeBuild project: Java-Temporary-Client-CI
Commit ID: 38157a9
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sqs-awslabs-ci · 2020-06-25T21:23:27Z

AWS CodeBuild CI Report

CodeBuild project: Java-Temporary-Client-CI
Commit ID: ae3dc3e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sqs-awslabs-ci · 2020-06-25T21:28:55Z

AWS CodeBuild CI Report

CodeBuild project: Java-Temporary-Client-CI
Commit ID: ae3dc3e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sqs-awslabs-ci · 2020-06-26T05:52:39Z

AWS CodeBuild CI Report

CodeBuild project: Java-Temporary-Client-CI
Commit ID: 3568df0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sqs-awslabs-ci · 2020-07-03T17:33:31Z

AWS CodeBuild CI Report

CodeBuild project: Java-Temporary-Client-CI
Commit ID: 7dd482b
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…lower bound

aws-sqs-awslabs-ci · 2020-07-03T19:40:33Z

AWS CodeBuild CI Report

CodeBuild project: Java-Temporary-Client-CI
Commit ID: f1b8b2f
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sqs-awslabs-ci · 2020-07-03T19:50:36Z

AWS CodeBuild CI Report

CodeBuild project: Java-Temporary-Client-CI
Commit ID: 45a460a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

robin-aws

Thanks for the additional test coverage! :)

robin-aws · 2020-07-03T20:22:46Z

src/test/java/com/amazonaws/services/sqs/AmazonSQSTemporaryQueuesClientIT.java

 }

 @Test
 public void createQueueWithUnsupportedAttributes() {
- try {
+ Assertions.assertThrows(IllegalArgumentException.class, () -> {


Awesome, thanks for applying assertThrows here :)

robin-aws · 2020-07-03T20:27:59Z

src/test/java/com/amazonaws/services/sqs/AmazonSQSTemporaryQueuesClientIT.java

+ AmazonSQSRequesterClientBuilder requesterBuilder;
+ if (idleQueueRetentionPeriod != null) {
+ requesterBuilder =
+ AmazonSQSRequesterClientBuilder.standard()
+ .withAmazonSQS(sqs)
+ .withInternalQueuePrefix(queueNamePrefix)
+ .withQueueRetentionPeriodSeconds(idleQueueRetentionPeriod);
+ } else {
+ requesterBuilder =
+ AmazonSQSRequesterClientBuilder.standard()
+ .withAmazonSQS(sqs)
+ .withInternalQueuePrefix(queueNamePrefix);
 }


Suggested change

AmazonSQSRequesterClientBuilder requesterBuilder;

if (idleQueueRetentionPeriod != null) {

requesterBuilder =

AmazonSQSRequesterClientBuilder.standard()

.withAmazonSQS(sqs)

.withInternalQueuePrefix(queueNamePrefix)

.withQueueRetentionPeriodSeconds(idleQueueRetentionPeriod);

} else {

requesterBuilder =

AmazonSQSRequesterClientBuilder.standard()

.withAmazonSQS(sqs)

.withInternalQueuePrefix(queueNamePrefix);

}

AmazonSQSRequesterClientBuilder requesterBuilder =

AmazonSQSRequesterClientBuilder.standard()

.withAmazonSQS(sqs)

.withInternalQueuePrefix(queueNamePrefix);

if (idleQueueRetentionPeriod != null) {

requesterBuilder = requesterBuilder.withQueueRetentionPeriodSeconds(idleQueueRetentionPeriod);

}

updated setup function

robin-aws · 2020-07-03T20:28:44Z

src/test/java/com/amazonaws/services/sqs/AmazonSQSTemporaryQueuesClientIT.java

+ client = AmazonSQSTemporaryQueuesClient.make(requesterBuilder);
+ }
+
+ private void createQueue(Long idleQueueRetentionPeriod) {


nit: Consider renaming this to something more descriptive, since its not just a helper creating a queue for further testing, but actually doing the important assertions itself. Perhaps createQueueShouldSetRetentionPeriod?

Renamed the function

robin-aws · 2020-07-03T20:31:09Z

src/main/java/com/amazonaws/services/sqs/AmazonSQSRequesterClientBuilder.java

@@ -17,6 +17,7 @@
 private int idleQueueSweepingPeriod = 5;
 private long queueHeartbeatInterval = AmazonSQSIdleQueueDeletingClient.HEARTBEAT_INTERVAL_SECONDS_DEFAULT;
 private TimeUnit idleQueueSweepingTimeUnit = TimeUnit.MINUTES;
+ private long queueRetentionPeriodSeconds = AmazonSQSTemporaryQueuesClientBuilder.QUEUE_RETENTION_PERIOD_SECONDS_DEFAULT;


Apologies for not spotting this earlier, but this should really be idleQueueRetentionPeriodSeconds in all these new places. The distinction has caused confusion for customers in the past. You can't change the existing public constants, but at least new references can be more precise.

updated queueRetentionPeriodSeconds to idleQueueRetentionPeriodSeconds in all the new places

aws-sqs-awslabs-ci · 2020-07-03T21:03:04Z

AWS CodeBuild CI Report

CodeBuild project: Java-Temporary-Client-CI
Commit ID: 980605e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

robin-aws · 2020-07-03T21:10:06Z

src/main/java/com/amazonaws/services/sqs/AmazonSQSTemporaryQueuesClientBuilder.java

@@ -5,6 +5,8 @@

 public class AmazonSQSTemporaryQueuesClientBuilder {

+ public final static long QUEUE_RETENTION_PERIOD_SECONDS_DEFAULT = 300;


Suggested change

public final static long QUEUE_RETENTION_PERIOD_SECONDS_DEFAULT = 300;

public final static long IDLE_QUEUE_RETENTION_PERIOD_SECONDS_DEFAULT = 300;

(and the references too of course :))

aws-sqs-awslabs-ci · 2020-07-03T21:21:36Z

AWS CodeBuild CI Report

CodeBuild project: Java-Temporary-Client-CI
Commit ID: 25ee02f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Allow configuration of IdleQueueRetentionPeriodSeconds on temporary q…

24a4cab

…ueue client

adam-aws requested a review from robin-aws June 19, 2020 20:12

robin-aws requested changes Jun 23, 2020

View reviewed changes

Idle Queue Retention to long and added junit 5

38157a9

adam-aws requested a review from robin-aws June 25, 2020 19:42

Merge branch 'master' into issue-38

ae3dc3e

adam-aws added 2 commits June 25, 2020 18:07

Added junit vintage for existing junit 4 tests

bdf3083

Refactored IT for temporary queues client

3568df0

Merge branch 'master' into issue-38

7dd482b

adam-aws and others added 2 commits July 3, 2020 15:25

Merge branch 'master' into issue-38

f1b8b2f

idleQueueRetentionPeriod no longer has upper bound limit so checking …

45a460a

…lower bound

robin-aws reviewed Jul 3, 2020

View reviewed changes

PR changes

980605e

robin-aws reviewed Jul 3, 2020

View reviewed changes

update constant name

25ee02f

robin-aws approved these changes Jul 3, 2020

View reviewed changes

adam-aws merged commit adcb1ae into awslabs:master Jul 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration of IdleQueueRetentionPeriodSeconds on temporary queue client #42

Allow configuration of IdleQueueRetentionPeriodSeconds on temporary queue client #42

adam-aws commented Jun 19, 2020

VeniXuan commented Jun 23, 2020

robin-aws left a comment

robin-aws Jun 23, 2020

adam-aws Jun 24, 2020

robin-aws Jun 23, 2020

adam-aws Jun 24, 2020

adam-aws commented Jun 24, 2020

aws-sqs-awslabs-ci commented Jun 25, 2020

aws-sqs-awslabs-ci commented Jun 25, 2020

aws-sqs-awslabs-ci commented Jun 26, 2020

aws-sqs-awslabs-ci commented Jul 3, 2020

aws-sqs-awslabs-ci commented Jul 3, 2020

aws-sqs-awslabs-ci commented Jul 3, 2020

robin-aws left a comment

robin-aws Jul 3, 2020

robin-aws Jul 3, 2020

adam-aws Jul 3, 2020

robin-aws Jul 3, 2020

adam-aws Jul 3, 2020

robin-aws Jul 3, 2020

adam-aws Jul 3, 2020

aws-sqs-awslabs-ci commented Jul 3, 2020

robin-aws Jul 3, 2020

aws-sqs-awslabs-ci commented Jul 3, 2020

		@@ -5,6 +5,8 @@

		public class AmazonSQSTemporaryQueuesClientBuilder {

		public final static long QUEUE_RETENTION_PERIOD_SECONDS_DEFAULT = 300;

	public final static long QUEUE_RETENTION_PERIOD_SECONDS_DEFAULT = 300;
	public final static long IDLE_QUEUE_RETENTION_PERIOD_SECONDS_DEFAULT = 300;

Allow configuration of IdleQueueRetentionPeriodSeconds on temporary queue client #42

Allow configuration of IdleQueueRetentionPeriodSeconds on temporary queue client #42

Conversation

adam-aws commented Jun 19, 2020

VeniXuan commented Jun 23, 2020

robin-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-aws commented Jun 24, 2020

AWS CodeBuild CI Report

aws-sqs-awslabs-ci commented Jun 25, 2020

AWS CodeBuild CI Report

aws-sqs-awslabs-ci commented Jun 25, 2020

AWS CodeBuild CI Report

aws-sqs-awslabs-ci commented Jun 26, 2020

AWS CodeBuild CI Report

aws-sqs-awslabs-ci commented Jul 3, 2020

AWS CodeBuild CI Report

aws-sqs-awslabs-ci commented Jul 3, 2020

AWS CodeBuild CI Report

aws-sqs-awslabs-ci commented Jul 3, 2020

AWS CodeBuild CI Report

robin-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-sqs-awslabs-ci commented Jul 3, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

aws-sqs-awslabs-ci commented Jul 3, 2020

AWS CodeBuild CI Report