-
Notifications
You must be signed in to change notification settings - Fork 28
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
IdleQueueHeartbeatInterval configurable from builder #23
Conversation
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.
Thanks for the contribution! LGTM aside from some minor tweaks.
|
||
|
||
public long getIdleQueueHeartbeatInterval() { | ||
return idleQueueHeartbeatInterval; |
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.
No tabs please! :) Weird that it's only this file affected AFAICT.
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.
Weird that only this file was indented with tabs, should be fixed now.
@@ -15,6 +15,7 @@ | |||
private Map<String, String> queueAttributes = Collections.emptyMap(); | |||
|
|||
private int idleQueueSweepingPeriod = 5; | |||
private long idleQueueHeartbeatInterval = 5; |
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 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!
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.
Removed the "idle" part of the variable and methods
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; |
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.
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.
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.
Moved the default into the builder.
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 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.
@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.
I finally had some time to address the feedback you left me, hope this helps :) |
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.
Thanks @hcalsos! I added a couple more nit-picky requests but it's almost there.
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; |
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 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
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.
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.
@@ -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) { |
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 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.
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.
Added separate issue concerning this
@@ -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; |
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 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.
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.
Changed the value to 1 as well as the comparison to '<'.
@@ -6,15 +6,18 @@ | |||
import java.util.Optional; | |||
import java.util.concurrent.TimeUnit; | |||
|
|||
import static com.amazonaws.services.sqs.AmazonSQSIdleQueueDeletingClient.HEARTBEAT_INTERVAL_SECONDS_DEFAULT; |
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: 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
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.
Removed the static reference
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.
Thanks, LGTM! Just holding off on approval until #41 is addressed so we can see the integration tests pass for this first.
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 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Changed the comment to take into consideration the configuration changes |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.