IdleQueueHeartbeatInterval configurable from builder #23

hcalsos · 2019-11-04T09:00:41Z

Issue #19 :

Description of changes:
IdleQueueHeartbeatInterval configurable from builder.

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

robin-aws

Thanks for the contribution! LGTM aside from some minor tweaks.

robin-aws · 2019-11-09T00:03:51Z

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

-
+
+ public long getIdleQueueHeartbeatInterval() {
+ return idleQueueHeartbeatInterval;


No tabs please! :) Weird that it's only this file affected AFAICT.

Weird that only this file was indented with tabs, should be fixed now.

robin-aws · 2019-11-09T00:05:46Z

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

@@ -15,6 +15,7 @@
 private Map<String, String> queueAttributes = Collections.emptyMap();

 private int idleQueueSweepingPeriod = 5;
+ private long idleQueueHeartbeatInterval = 5;


Here and throughout, I'd take out the "idle" part and just go with queueHeartbeatInterval, since the whole point is that queues with heartbeats aren't idle!

Removed the "idle" part of the variable and methods

robin-aws · 2019-11-09T00:08:33Z

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

 super(sqs);

 if (queueNamePrefix.isEmpty()) {
 throw new IllegalArgumentException("Queue name prefix must be non-empty");
 }
 this.queueNamePrefix = queueNamePrefix;
+ this.HEARTBEAT_INTERVAL_SECONDS = heartbeatInterval != null ? heartbeatInterval : 5;


Can you define the default as a constant in the builder and reference it here instead of duplicating? I get that you're maintaining the constructor signature just to be safe.

Moved the default into the builder.

I realized after suggesting this that AmazonSQSIdleQueueDeletingClient doesn't have a builder. Can we define the default on this class instead and have both the requester client builder and responder client builder refer to it there?

We should also check for a minimum value here. Setting this to zero would mean a heartbeat for every client API call, every though some users might think it turns off heartbeats.

And finally (sorry to be picky - again I should have caught this earlier), can we change the parameter name to hearbeatIntervalSeconds everywhere? That will help it be more self-documenting.

robin-aws · 2020-04-06T16:24:06Z

@hcalsos would you be able to make the small requested edits on this one? We've had a few requests for this feature.

Removed tabs in 1 file. Removed idle from queueSweepingPeriod variable Moved default heartbeat interval to builder as constant.

hcalsos · 2020-05-12T06:27:49Z

@hcalsos would you be able to make the small requested edits on this one? We've had a few requests for this feature.

I finally had some time to address the feedback you left me, hope this helps :)

robin-aws

Thanks @hcalsos! I added a couple more nit-picky requests but it's almost there.

robin-aws · 2020-05-12T16:59:45Z

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

 super(sqs);

 if (queueNamePrefix.isEmpty()) {
 throw new IllegalArgumentException("Queue name prefix must be non-empty");
 }
 this.queueNamePrefix = queueNamePrefix;
+ this.HEARTBEAT_INTERVAL_SECONDS = heartbeatInterval != null ? heartbeatInterval : 5;


I realized after suggesting this that AmazonSQSIdleQueueDeletingClient doesn't have a builder. Can we define the default on this class instead and have both the requester client builder and responder client builder refer to it there?

We should also check for a minimum value here. Setting this to zero would mean a heartbeat for every client API call, every though some users might think it turns off heartbeats.

And finally (sorry to be picky - again I should have caught this earlier), can we change the parameter name to hearbeatIntervalSeconds everywhere? That will help it be more self-documenting.

…ue, moved default

robin-aws

Thanks for taking on finishing this PR @adam-aws! I just added a couple more nits but they're easy to fix.

I do want to make sure we get the integration tests running in CI before we merge this - we'll keep working together on getting that up and running ASAP.

robin-aws · 2020-06-17T20:20:48Z

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

@@ -278,7 +296,7 @@ private void heartbeatToQueueIfNecessary(String queueUrl) {
 QueueMetadata queueMetadata = queues.get(queueUrl);
 if (queueMetadata != null) {
 Long lastHeartbeat = queueMetadata.heartbeatTimestamp;
- if (lastHeartbeat == null || (System.currentTimeMillis() - lastHeartbeat) > 2 * HEARTBEAT_INTERVAL_SECONDS) {
+ if (lastHeartbeat == null || (System.currentTimeMillis() - lastHeartbeat) > 2 * heartbeatIntervalSeconds) {


I just noticed there's an existing bug here, comparing milliseconds on the LHS to seconds on the right. Let's log a separate issue for that - reveals a testing gap as well.

Added separate issue concerning this

robin-aws · 2020-06-17T20:24:46Z

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

@@ -70,10 +70,10 @@
 public static final String IDLE_QUEUE_RETENTION_PERIOD = "IdleQueueRetentionPeriodSeconds";
 public static final long MINIMUM_IDLE_QUEUE_RETENTION_PERIOD_SECONDS = 1;
 public static final long MAXIMUM_IDLE_QUEUE_RETENTION_PERIOD_SECONDS = TimeUnit.MINUTES.toSeconds(5);
-
+ public static final long HEARTBEAT_INTERVAL_SECONDS_DEFAULT = 5;
+ public static final long HEARTBEAT_INTERVAL_SECONDS_MIN_VALUE = 0;


I would make this 1 and change the comparison from a <= to a '<'. Since this is a discrete value I think making the "minimum" a valid value is easier to understand.

Changed the value to 1 as well as the comparison to '<'.

robin-aws · 2020-06-17T20:35:24Z

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

@@ -6,15 +6,18 @@
 import java.util.Optional;
 import java.util.concurrent.TimeUnit;

+import static com.amazonaws.services.sqs.AmazonSQSIdleQueueDeletingClient.HEARTBEAT_INTERVAL_SECONDS_DEFAULT;


nit: I don't think a single reference is worth a static reference, and probably better to be clear at the point of reference where this is coming from

Removed the static reference

… reference

robin-aws

Thanks, LGTM! Just holding off on approval until #41 is addressed so we can see the integration tests pass for this first.

robin-aws · 2020-06-23T22:36:40Z

I just noticed we refer to the hard-coded value here as well: https://github.com/awslabs/amazon-sqs-java-temporary-queues-client/blob/master/src/main/java/com/amazonaws/services/sqs/AmazonSQSIdleQueueDeletingClient.java#L61

adam-aws · 2020-06-23T22:59:39Z

AWS CodeBuild CI Report

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

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

adam-aws · 2020-06-23T23:01:05Z

I just noticed we refer to the hard-coded value here as well: https://github.com/awslabs/amazon-sqs-java-temporary-queues-client/blob/master/src/main/java/com/amazonaws/services/sqs/AmazonSQSIdleQueueDeletingClient.java#L61

Changed the comment to take into consideration the configuration changes

aws-sqs-awslabs-ci · 2020-06-25T20:01:46Z

AWS CodeBuild CI Report

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

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

IdleQueueHeartbeatInterval configurable from builder

d5ef6eb

robin-aws requested changes Nov 9, 2019

View reviewed changes

Made some changes after PR comments:

9917193

Removed tabs in 1 file. Removed idle from queueSweepingPeriod variable Moved default heartbeat interval to builder as constant.

robin-aws requested changes May 12, 2020

View reviewed changes

Applied small tweaks, changed parameter name, added check for min val…

499bc0d

…ue, moved default

robin-aws reviewed Jun 17, 2020

View reviewed changes

Changed HEARTBEAT_INTERVAL_SECONDS_MIN_VALUE value and removed static…

ef59f4d

… reference

adam-aws added this to the Release 1.0.4 milestone Jun 18, 2020

robin-aws requested changes Jun 23, 2020

View reviewed changes

Comment change for configurable heartbeating mechanism

156f531

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

Merge branch 'master' into master

9c1128c

robin-aws approved these changes Jun 25, 2020

View reviewed changes

adam-aws merged commit e6ffd9a into awslabs:master Jun 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IdleQueueHeartbeatInterval configurable from builder #23

IdleQueueHeartbeatInterval configurable from builder #23

hcalsos commented Nov 4, 2019

robin-aws left a comment

robin-aws Nov 9, 2019

hcalsos May 12, 2020

robin-aws Nov 9, 2019

hcalsos May 12, 2020

robin-aws Nov 9, 2019

hcalsos May 12, 2020

robin-aws May 12, 2020

robin-aws commented Apr 6, 2020

hcalsos commented May 12, 2020

robin-aws left a comment

robin-aws May 12, 2020

robin-aws left a comment

robin-aws Jun 17, 2020

adam-aws Jun 17, 2020

robin-aws Jun 17, 2020

adam-aws Jun 17, 2020

robin-aws Jun 17, 2020

adam-aws Jun 17, 2020

robin-aws left a comment

robin-aws commented Jun 23, 2020

adam-aws commented Jun 23, 2020

adam-aws commented Jun 23, 2020

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



		public long getIdleQueueHeartbeatInterval() {
		return idleQueueHeartbeatInterval;

IdleQueueHeartbeatInterval configurable from builder #23

IdleQueueHeartbeatInterval configurable from builder #23

Conversation

hcalsos commented Nov 4, 2019

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

robin-aws commented Apr 6, 2020

hcalsos commented May 12, 2020

robin-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

robin-aws left a comment

Choose a reason for hiding this comment

robin-aws commented Jun 23, 2020

adam-aws commented Jun 23, 2020

AWS CodeBuild CI Report

adam-aws commented Jun 23, 2020

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

AWS CodeBuild CI Report