doc: rate_limit uses max_age, not max-age - #13510
Conversation
The YAML parser reads max_age with an underscore, in three places: the queue node (limiter.h), the ip-rep node and its perma-block sub-node (ip_reputation.cc). Every mention in the documentation spelled it max-age, including the three worked examples, so a configuration copied from the docs left all three at their default of 0 and silently disabled queue expiry and both IP-reputation aging paths. The --maxage pparam description also referred to a "max-age" that matches neither the option nor the key, so it now just says maximum age.
JosiahWI
left a comment
There was a problem hiding this comment.
This is great. Could you add a mention of the historical reason for the naming inconsistency to the documentation, so that future developers understand that naming it max-age could activate previously inert configurations?
Clarify documentation regarding the 'max-age' option and its behavior in previous versions.
Added a note to the documentation as a separate commit. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (3)
doc/admin-guide/plugins/rate_limit.en.rst:96
- The
--maxageoption description says the value is in seconds, but the implementation parses it as milliseconds (seetxn_limiter.ccwhere--maxageis stored asstd::chrono::milliseconds). This should be corrected so operators don’t configure the wrong delay-queue expiry.
An optional maximum age for how long a transaction can sit in the delay queue.
The value (default 0) is the age in seconds.
doc/admin-guide/plugins/rate_limit.en.rst:217
- In the queue section, the sentences “The size is default to …” / “The max_age is …” are ungrammatical; they should read “defaults to” for clarity.
The queue option can include a `size` and a `max_age` option. The size is
default to ``UINT_MAX``, which is essentially unlimited. The max_age is
default to ``0``, which means no age limit.
doc/admin-guide/plugins/rate_limit.en.rst:281
- The new note claims the code ignores
max-age“in all versions” of the plugin, which is hard to substantiate from the current tree. Consider rephrasing to the verifiable behavior: the plugin expectsmax_ageand ignoresmax-age.
Note that this option was incorrectly documented as ``max-age`` with a hyphen in previous
versions. The code ignores ``max-age`` in all versions of this plugin. We have chosen
to keep the behavior the same and update the documentation, so that previously inert
configurations don't activate unexpectedly with an upgrade.
|
[approve ci rocky] |
The pparam path stores --maxage as std::chrono::milliseconds (txn_limiter.cc), while the YAML path stores max_age as std::chrono::seconds (limiter.h). The documentation described both as seconds, so a --maxage value was off by a factor of 1000. Document the units as they behave and note the discrepancy, rather than changing the expiry of existing configurations. Also state the unit on the YAML side, which was unstated, and fix the "is default to" wording.
|
[approve ci rocky] |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
doc/admin-guide/plugins/rate_limit.en.rst:286
- This paragraph claims the code ignores
max-agein all versions of the plugin, which is hard to verify and could become incorrect if older versions behaved differently. Suggest rewording to describe the current parser behavior without asserting historical guarantees.
Note that this option was incorrectly documented as ``max-age`` with a hyphen in previous
versions. The code ignores ``max-age`` in all versions of this plugin. We have chosen
to keep the behavior the same and update the documentation, so that previously inert
configurations don't activate unexpectedly with an upgrade.
doc/admin-guide/plugins/rate_limit.en.rst:101
- The note about differing units uses an absolute/historical claim ("have always differed") and the phrase "rather than made consistent" is grammatically awkward. Consider wording this in terms of current behavior to avoid making unverifiable statements about past versions, and tighten the phrasing.
This issue also appears on line 283 of the same file.
Note that the equivalent YAML setting, ``max_age`` under a ``queue`` node, is in
seconds. The two configuration paths have always differed by a factor of 1000, so
the units are documented here as they behave rather than made consistent, to avoid
changing the expiry of existing configurations.
|
[approve ci format rat docs freebsd] |
The rate_limit YAML parser reads
max_agewith an underscore, in three places:queuenode —plugins/experimental/rate_limit/limiter.hip-repnode —plugins/experimental/rate_limit/ip_reputation.ccperma-blocksub-node — same fileEvery mention in the documentation spelled it
max-age, including all threeworked examples. A configuration copied from the docs therefore left
_max_ageat its default of
0in each case, which silently disables queue expiry (theexpiry pass in
sni_queue_cont()is gated onmax_age() > std::chrono::milliseconds::zero()) and both IP-reputation agingpaths.
This changes the documentation rather than the parser, so that configurations
which are inert today stay inert on upgrade instead of suddenly beginning to
expire queue entries or age out LRU entries with whatever value the operator
wrote.
The
--maxagepparam description also referred to amax-agethat matchesneither the option name nor the YAML key, so it now just says "maximum age".
max-agewas the only key in the documentation that did not match the parser;ip-repandperma-blockreally are hyphenated in the code.