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

Added POLL_DELAY environment variable to UserConfiguration class #3421

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

abujeda
Copy link
Contributor

@abujeda abujeda commented Mar 14, 2024

Added POLL_DELAY environment variable to UserConfiguration class.

This change is backwards compatible with the environment variable.

Fixes #3413

@johrstrom
Copy link
Contributor

I'm not sure if it should be a profile based thing. Generally folks want to control this so that squeue calls happen less frequently, so I'm not sure what the advantage here is to have one profile at 10 seconds and another at 20 for example.

@abujeda
Copy link
Contributor Author

abujeda commented Mar 14, 2024

I always tend to add all properties to the UserConfiguration class to make them available in profiles and, in this case, because it already supports ENV variables.

Will move it to the main Configuration object.

@abujeda
Copy link
Contributor Author

abujeda commented Mar 14, 2024

Moved to Configuration object.

Comment on lines 371 to 378
def sessions_poll_delay
poll_delay = ENV['POLL_DELAY']
poll_delay.nil? ? config.fetch(:sessions_poll_delay, '10000') : poll_delay.to_s
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a couple things here.

First can we prepend bc_ to this. That'll follow other configuration names so folks can see they all relate to batch_connect.

Secondly - I'm thinking we need some sort of check like casting to_i to be sure it's an integer. I also think that 10000 should be the default and likely minimum too. I'd hate to see someone configure 1 and just blow up their system lol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is better. I completed the updates

Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround on this!

@johrstrom johrstrom merged commit e580bf6 into OSC:master Mar 15, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replace POLL_DELAY with actual configuration
3 participants