Skip to content

Conversation

@TreyE
Copy link
Contributor

@TreyE TreyE commented Oct 21, 2022

After examination, we've discovered that Puma instance (i.e. user-facing web-UI) responsiveness suffers significantly when the system is under heavy asynchronous load (such as during redeterminations).

The reason behind this is that the event source AMQP subscribers are also started on the Puma webserver instances, instead of only on listener instances. This results in AMQP subscribers consuming messages, doing work, and 'stealing' resources from the Puma webserver threads on the same instance.

This update enables a DevOps engineer to opt-in to the creation of asynchronous AMQP subscribers by setting an environment variable ES_HOSTING_MODE to a value of listener. Only when this environment variable is explicitly set, and only when it is explicitly set to the value of listener, will AMQP subscribers be created.

Important to note is:

  1. The default for running AMQP subscribers is now opt-in - tests and local docker environments will need to be updated to reflect this.
  2. All applications which use event source now have access to this functionality, without further developer effort.

@polographer
Copy link

polographer commented Oct 27, 2022

this would be awesome, an unwanted request/comment; have you thought about changing the name of the flag to be more generic? (instead of "web", or ad another mode like "console"); what I'm thinking is that "rails c" also initializes 'event source' and converts itself into another "worker" (it also takes more time to boot).

If we merge it as it is we can do it via ES_HOSTING_MODE=web rails c, but the wording looks a little off, I was thinking something more generic like ES_THREADS=false rails c or ES_CONSUMER=false rails c, that way we can reuse it in more places and look more generic

@TreyE
Copy link
Contributor Author

TreyE commented Oct 27, 2022

this would be awesome, an unwanted request/comment; have you thought about changing the name of the flag to be more generic? (instead of "web", or ad another mode like "console"); what I'm thinking is that "rails c" also initializes 'event source' and converts itself into another "worker" (it also takes more time to boot).

If we merge it as it is we can do it via ES_HOSTING_MODE=web rails c, but the wording looks a little off, I was thinking something more generic like ES_THREADS=false rails c or ES_CONSUMER=false rails c, that way we can reuse it in more places and look more generic

Main thing here is we do want to start these in console and dev mode. When running Puma in production is really the only time we don't want to start them. Web seemed like the right name for me, but other than that we could call it whatever.

@TreyE
Copy link
Contributor Author

TreyE commented Oct 28, 2022

I have updated the flag to be named 'publisher' as it is not only in the Puma or 'web' context that we desire this behaviour. We want it when legacy ACAPI listeners run, as well as an any kind of batch run.

@TreyE TreyE requested a review from raghuramg October 28, 2022 16:04
@TreyE TreyE force-pushed the puma_service_optimization branch from 226f1e9 to 5cfb423 Compare August 1, 2023 17:07
@TreyE TreyE force-pushed the puma_service_optimization branch from 5cfb423 to c11aa03 Compare October 16, 2023 19:04
@TreyE TreyE added the enhancement New feature or request label Oct 16, 2023
@TreyE TreyE force-pushed the puma_service_optimization branch from ae7203a to e9ef3d1 Compare October 16, 2023 19:49
Copy link

@kristinmerbach kristinmerbach left a comment

Choose a reason for hiding this comment

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

LGTM

def server_key=(value)
@server_key = value&.to_sym
end

Copy link
Member

Choose a reason for hiding this comment

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

Adding a comment to just block the merge until we finalize the date we want to merge.

@TreyE
Copy link
Contributor Author

TreyE commented May 19, 2025

I'm likely to close this and submit it in a different form. I don't care for how I've chosen nor am enforcing some of the defaults - I think it clashes with both the current implementation and expected deployment, development, and testing defaults of event source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants