Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

some duration settings in the config file do not allow a unit suffix #12756

Open
richvdh opened this issue May 17, 2022 · 3 comments
Open

some duration settings in the config file do not allow a unit suffix #12756

richvdh opened this issue May 17, 2022 · 3 comments
Assignees
Labels
A-Config Configuration, or the documentation thereof O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@richvdh
Copy link
Member

richvdh commented May 17, 2022

The config documentation claims 'Configuration options that take a time period can be set using a number followed by a letter', but this is not universally true. Examples include:

  • background_updates.background_update_duration_ms
  • background_updates.sleep_duration_ms
@richvdh
Copy link
Member Author

richvdh commented May 17, 2022

(tangentially related: #5584 (comment))

@babolivier babolivier added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label May 17, 2022
@H-Shay H-Shay self-assigned this May 17, 2022
@MadLittleMods MadLittleMods added the A-Config Configuration, or the documentation thereof label Jun 3, 2022
@DMRobertson DMRobertson added S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. O-Uncommon Most users are unlikely to come across this or unexpected workflow and removed T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. labels Aug 25, 2022
@dklimpel
Copy link
Contributor

If these two values are also parsed by parse_duration,

def parse_duration(value: Union[str, int]) -> int:

would it make sense to also parse a character string such as ms?

Perhaps also add that integer values automatically means miliseconds?

Configuration options that take a time period can be set using a number
followed by a letter. Letters have the following meanings:
* `s` = second
* `m` = minute
* `h` = hour
* `d` = day
* `w` = week
* `y` = year
For example, setting `redaction_retention_period: 5m` would remove redacted
messages from the database after 5 minutes, rather than 5 months.

@richvdh
Copy link
Member Author

richvdh commented Sep 30, 2022

If these two values are also parsed by parse_duration,

they are not parsed by parse_duration. This is the problem here. We should add new config settings background_update_duration, etc, which are parsed by parse_duration, and deprecate the old settings.

would it make sense to also parse a character string such as ms?

probably yes, but that doesn't solve this issue.

Perhaps also add that integer values automatically means miliseconds?

again yes that would be helpful, but again doesn't solve this particular problem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Config Configuration, or the documentation thereof O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

No branches or pull requests

6 participants