Skip to content
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

Allow more SQS settings to be set by env var #20

Merged
merged 3 commits into from
Nov 23, 2022

Conversation

mlarraz
Copy link
Collaborator

@mlarraz mlarraz commented Nov 1, 2022

Add a few optional settings that can be configured by environment variable:

  • SCOUT_SQS_MAX_NUMBER_OF_MESSAGES - Max number of SQS messages to fetch at once
  • SCOUT_SQS_WAIT_TIME_SECONDS - Max seconds to wait for an SQS message per poll
  • SCOUT_SQS_VISIBILITY_TIMEOUT - How long to hide an SQS message after receiving it

These can also be set in the YAML config under a new sqs key:

sqs:
  max_number_of_messages: 10
  wait_time_seconds: 30
  visibility_timeout: 30

@mlarraz mlarraz marked this pull request as ready for review November 2, 2022 22:19
main.go Outdated
signals chan os.Signal
app *cli.App
signals chan os.Signal
sqsDefaults sqsSettings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the SQS client is really owned by Queue, I think it'd be cleaner to add these properties onto the Config object and let Queue handle passing them down to SqsClient at the time it initializes it. Passing them under-the-table with this global variable is breaking the encapsulation.

I don't have a strong opinion on if these need to come from the config file as opposed to coming in from ENV vars, but if you add these sqsSettings into the config object (e.g. config.SQS), then I think we'll get the ability to set them from the config file for free which will fit better into the way that Enova uses Scout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if the updated commit does what you're thinking. Ideally I want to make the config file entirely optional and allow everything to be set via environment

Copy link
Contributor

@rnubel rnubel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the delay here. I've set up email notifications now so I should be more responsive now.

Clever trick with the embedding! I added suggestions for yaml tags so the values could be parsed out of the config in a uniform fashion -- if that breaks the embedding, though, we can move forward without them.

config.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
@rnubel rnubel merged commit b910652 into enova:main Nov 23, 2022
@mlarraz mlarraz deleted the sqs_settings branch December 4, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants