Jk/cumulus 3020 #3800
Jk/cumulus 3020 #3800
Conversation
tf-modules/cumulus/variables.tf
Outdated
| @@ -626,3 +626,9 @@ variable "report_sns_topic_subscriber_arns" { | |||
| default = null | |||
| description = "Account ARNs to supply to report SNS topics policy with subscribe action" | |||
| } | |||
|
|
|||
| variable "sf_event_sqs_lambda_timeout" { | |||
There was a problem hiding this comment.
I saw this ticket in review for the sprint board, and didn't know it was already approved, but looking at it, instead of adding this variable like this, can we not just add the check for SfEventSqsLambda timeout like we do for https://github.com/nasa/cumulus/blob/master/tf-modules/archive/variables.tf#L219 here:
? seems easier, but this timeout looks a bit different than the othersThere was a problem hiding this comment.
@Nnaga1 that's more or less what @jennyhliu asked. I have strong reservations about if we should really lump internal lambdas into a generic user configurable timeout for framework AND workflow lambdas, given it's difficult to document how modifications to this could cause serious negative impacts and/or support obligations, but I'm willing to go with the team's opinion on it.
We definitely don't have documentation on this configuration, option other than a mention in 'task_configuration.md', which seems inadequate long-term if we've expanded the scope of this configuration value.
Apologies that discussion wasn't captured here, this ticket has just been on the back burner since the start of the sprint.
There was a problem hiding this comment.
@npauzenga @Nnaga1 @jennyhliu take a look at e002598
I'll add a documentation ticket to cover the doc concern above prior to closing this ticket.
There was a problem hiding this comment.
Apologies for missing the back half of this conversation. I thought we'd agreed to NOT include this as part of the lambda_timeouts. The sfevent lambda is a central framework component that (ideally) we don't want users configuring in this way if they don't have to. At least that's my impression. I'm not sure what the consequences could be if this value changes.
Having this config in a separate, explicit, mildly buried variable for only users that really really know what they're doing was appealing. Giving the keys over in an object like lambda_timeouts makes it easier to see and adjust, which I'm not sure is what we want?
There was a problem hiding this comment.
tl;dr I kind of liked the initial approach and am a little concerned about this one but, like Jonathan, can be persuaded 😄
There was a problem hiding this comment.
For posterity: discussed this in a huddle on 4 October and agreed with the current PR + scoped work to address and a arch meeting topic to discuss the approach with the team.
Summary: Summary of changes
Addresses CUMULUS-3020
PR Checklist