-
Notifications
You must be signed in to change notification settings - Fork 608
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
Rework KariusDx:config-cleanup-wait #302
Conversation
1. Rename config.CleanupWaitDuration to config.TaskCleanupWaitDuration to be more explicit about its intent. 2. Add a minimum threshold for the cleanup time. Accepting low values such as '1 us' causes the agent to deadlock. 3. Fix a bug in the engine code which could cause a deadlock when using en empty config.Config object in the engine. The waitEvent method will deadlock when waiting for cleanup if the cleanup go routine exits prematurely. A check has been added to the event duation to fix this. 4. Add functional tests for task cleanup.
…nvironment variable
1. Use the DefaultTaskCleanupWaitDuration while constructing default config. 2. Edit the debug message to fix inconsistent casing.
taskCleanupWaitDuration := parseEnvVariableDuration("ECS_ENGINE_TASK_CLEANUP_WAIT_DURATION") | ||
// If a value has been set for taskCleanupWaitDuration and the value is less than the minimum allowed cleanup duration, | ||
// print a warning and override it | ||
if taskCleanupWaitDuration < minimumTaskCleanupWaitDuration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic should really be in NewConfig
so it catches errors in FileConfig
as well. Also, if you want to make FileConfig
, EnvironmentConfig
, and EC2MetadataConfig
private as well, I wouldn't complain. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I fell into the trap. Will fix this.
Also made FileConfig, EnvironmentConfig, and EC2MetadataConfig private.
👍 |
merged |
Addressed comments in #286. Also added functional tests and some validations.