Add new env var for sqsMsgVisibilityTimeoutSec #1220
Conversation
|
Hi @LikithaVemulapalli @stevegocoding, could you help me take a look? |
pkg/monitor/sqsevent/sqs-monitor.go
Outdated
| func (m SQSMonitor) receiveQueueMessages(qURL string) ([]*sqs.Message, error) { | ||
| visibilityTimeout := m.SqsMsgVisibilityTimeoutSec | ||
| if visibilityTimeout <= 0 { | ||
| visibilityTimeout = 20 |
There was a problem hiding this comment.
can we import the default constant from config instead of directly adding 20 here for single source of truth
pkg/config/config.go
Outdated
| flag.BoolVar(&config.UseAPIServerCacheToListPods, "use-apiserver-cache", getBoolEnv(useAPIServerCache, false), "If true, leverage the k8s apiserver's index on pod's spec.nodeName to list pods on a node, instead of doing an etcd quorum read.") | ||
| flag.IntVar(&config.HeartbeatInterval, "heartbeat-interval", getIntEnv(heartbeatIntervalKey, -1), "The time period in seconds between consecutive heartbeat signals. Valid range: 30-3600 seconds (30 seconds to 1 hour).") | ||
| flag.IntVar(&config.HeartbeatUntil, "heartbeat-until", getIntEnv(heartbeatUntilKey, -1), "The duration in seconds over which heartbeat signals are sent. Valid range: 60-172800 seconds (1 minute to 48 hours).") | ||
| flag.IntVar(&config.SqsMsgVisibilityTimeoutSec, "sqs-msg-visibility-timeout-sec", getIntEnv(sqsMsgVisibilityTimeoutSecConfigKey, sqsMsgVisibilityTimeoutSecDefault), "Duration in seconds that a message is hidden from other consumers after being retrieved from the SQS queue by sqs-monitor.") |
There was a problem hiding this comment.
I believe we should add validation for this SQS visibility timeout field, the range can be from 0 to 43200 sec which is 12 hrs, SQS throws an error after 12 hrs, so could you include the range in all the comments for this field and also validation to throw error if config.SqsMsgVisibilityTimeoutSec < 0 || config.SqsMsgVisibilityTimeoutSec > 43200 and also the default per this doc is 30 seconds.
For reference: https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-visibility-timeout.html
| | `topologySpreadConstraints` | [Topology Spread Constraints](https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/) for pod scheduling. Useful with a highly available deployment to reduce the risk of running multiple replicas on the same Node | `[]` | | ||
| | `heartbeatInterval` | The time period in seconds between consecutive heartbeat signals. Valid range: 30-3600 seconds (30 seconds to 1 hour). | `-1` | | ||
| | `heartbeatUntil` | The duration in seconds over which heartbeat signals are sent. Valid range: 60-172800 seconds (1 minute to 48 hours). | `-1` | | ||
| | `sqsMsgVisibilityTimeoutSec` | Duration in seconds that a message is hidden from other consumers after being retrieved from the SQS queue by sqs-monitor. | `20` | |
There was a problem hiding this comment.
update comment here as well after including the range
2 participants
Issue #, if available:
#1205
Description of changes:
Add new env var for sqsMsgVisibilityTimeoutSec to SQS mode.
Quick revisit how NTH listen to ASG launch event:
So we better let SQS visibility timeout become configurable. Which can mitigate the 1205 issue.
Solution:
How you tested your changes:
Environment (Linux / Windows):
Kubernetes Version:
Test1:
Test2:
Test3:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.