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

For delivery service parameter docs explain parent_retry deprecation #7120

Merged
merged 5 commits into from
Dec 16, 2022

Conversation

traeak
Copy link
Contributor

@traeak traeak commented Oct 11, 2022

The t3c logic currently ignores the parent_retry ds parameter and instead examines the use of parameters

  • max_simple_retries
  • simple_server_retry_responses
  • max_unavailable_server_retries
  • unavailable_server_retry_responses

in order to infer what value to use for parent_retry in parent.config

Setting either of the above pairs to '0', '""' will disable, otherwise default is always "both".


Which Traffic Control components are affected by this PR?

  • Documentation

What is the best way to verify this PR?

If this is a bugfix, which Traffic Control versions contained the bug?

PR submission checklist

@traeak traeak marked this pull request as ready for review October 11, 2022 14:28
@traeak traeak changed the title For parent_retry docs explain parent_retry deprecation For parent parameter docs explain parent_retry deprecation Oct 11, 2022
@traeak traeak changed the title For parent parameter docs explain parent_retry deprecation For delivery service parameter docs explain parent_retry deprecation Oct 11, 2022
@mitchell852 mitchell852 added documentation related to documentation improvement The functionality exists but it could be improved in some way. labels Oct 11, 2022
@traeak traeak force-pushed the parent_retry_docs branch from c855bbf to eb66f1f Compare November 9, 2022 19:36
@ocket8888 ocket8888 added the low impact affects only a small portion of a CDN, and cannot itself break one label Nov 9, 2022
@ocket8888 ocket8888 self-assigned this Dec 13, 2022

The above :term:`Parameters` are supported for ``first``, ``inner`` and ``last`` tiers by specifying prefixes ``first.``, ``inner.`` and ``last.``, applicable to both topology and non topology. This allows fine tuning of marking parents "down" and retry behavior inside a CDN.

.. deprecated:: The ``mso.`` prefix is deprecated. ``last.`` prefix should be preferred although no prefix can also be used.

.. warning:: The `simple_retry_response_codes`` :term:`Parameter` has no apparent, possible use according to the :abbr:`ATS (Apache Traffic Server)` `parent.config documentation <https://docs.trafficserver.apache.org/en/9.1.x/admin-guide/files/parent.config.en.html>`_. Whether or not it has any effect - let alone the *intended* effect - is not known, and its use is therefore strongly discouraged.
.. deprecated:: The parent_retry parameter is now inferred from the `simple retry` and `unavailable server retry` parameters. To disable `simple retries` associate parameter ``max_simple_retries`` of ``0`` and ``max_simple_retry_responses`` of ``""``. Similarly "unavailable server retries" may also be disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

This lists parent_retry as deprecated, but should also mention last.parent_retry, unless you'd rather give that its own deprecation notice.

parent_retry should probably be monospace.

"simple retry" is probably meant to be "simple_retry" - using a single grave accent (or "backtick": `) to surround a body of text emphasizes it by default, not monospace as in Markdown. You need two. Also missing the underscore.

"unavailable server retry parameters" should probably be "unavailable_server_retries, unavailable_server_retry_response_codes, and max_unavailable_server_retries Parameters" (note the title-casing on "Parameters").

Extra space between "." and "To", not a big deal but just looks weird IMO.

"To disable simple retries associate parameter max_simple_retries of 0 and max_simple_retry_responses of "". Similarly 'unavailable server retries' may also be disabled." should probably be

To disable "simple retries" for a :term:`Profile`, set the Value of its ``max_simple_retries`` :term:`Parameter` to ``0``, and the Value of its ``max_simple_retry_responses`` :term:`Parameter` to an empty string. "Unavailable server retries" may disabled in much the same way, using the analogous :term:`Parameters`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, last.parent_retry isn't even in this file. That deprecation notice should be wherever the Parameter is documented. But it wouldn't surprise me if it wasn't at all outside of the quick-how-to, so if that's the case, just disregard that part of the comment.

.. warning:: The `simple_retry_response_codes`` :term:`Parameter` has no apparent, possible use according to the :abbr:`ATS (Apache Traffic Server)` `parent.config documentation <https://docs.trafficserver.apache.org/en/9.1.x/admin-guide/files/parent.config.en.html>`_. Whether or not it has any effect - let alone the *intended* effect - is not known, and its use is therefore strongly discouraged.
.. deprecated:: The parent_retry parameter is now inferred from the `simple retry` and `unavailable server retry` parameters. To disable `simple retries` associate parameter ``max_simple_retries`` of ``0`` and ``max_simple_retry_responses`` of ``""``. Similarly "unavailable server retries" may also be disabled.

.. impl-detail:: With Apache Traffic Server 8.1.x the `simple_retry_response_codes` setting is not available.
Copy link
Contributor

Choose a reason for hiding this comment

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

"simple_retry_response_codes" is probably meant to be "simple_retry_response_codes".

.. deprecated:: The parent_retry parameter is now inferred from the `simple retry` and `unavailable server retry` parameters. To disable `simple retries` associate parameter ``max_simple_retries`` of ``0`` and ``max_simple_retry_responses`` of ``""``. Similarly "unavailable server retries" may also be disabled.

.. impl-detail:: With Apache Traffic Server 8.1.x the `simple_retry_response_codes` setting is not available.
.. impl-detail:: With Apache Traffic Server 9.1.x `unavailable_server_retry_response_codes` are limited to 5xx responses and `simple_retry_response_codes` are limited to 4xx.
Copy link
Contributor

Choose a reason for hiding this comment

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

"unavailable_server_retry_response_codes" is probably meant to be "unavailable_server_retry_response_codes".

"simple_retry_response_codes" is probably meant to be "simple_retry_response_codes".


.. impl-detail:: With Apache Traffic Server 8.1.x the `simple_retry_response_codes` setting is not available.
.. impl-detail:: With Apache Traffic Server 9.1.x `unavailable_server_retry_response_codes` are limited to 5xx responses and `simple_retry_response_codes` are limited to 4xx.
.. impl-detail:: Apache Traffic Server 9.2.x allows more flexibility with 4xx and 5xx codes available for use with `simple_retry_response_codes`.
Copy link
Contributor

Choose a reason for hiding this comment

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

"simple_retry_response_codes" is probably meant to be "simple_retry_response_codes".

| parent_retry | `parent_retry`_ | Sets whether the :term:`cache servers` will use "simple retries", |
| | | "unavailable server retries", or both. |
| | | "unavailable server retries", or both. Deprecated (see below). |
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "see below", the docs guidelines encourage using actual links, to make the flow of a document less fragile:

"When referencing information in another section on the same page, please do not refer to the current placement of the referenced content relative to the referencing content. For example, instead of 'as discussed below', use 'as discussed in Terms'."

This is because tables, figures, asides and some others are all "floating environments", and the output format is not strictly required to replicate the exact placement of those floating environments with respect to surrounding text. For example, in the PDF version of the docs, this table would likely be pushed to the top of the next available page, which could cause the text to which it refers to actually precede it. In that case, "above" and "below" make no sense anyway, because the entire section isn't on a single page.

You could use a footnote, or simply mark it as "deprecated" and let the admonition speak for itself on the matter. Or you could try to link it, but I suspect you'll quickly decide that's too annoying to bother with - because it kinda is.

@traeak traeak force-pushed the parent_retry_docs branch 2 times, most recently from 06c5ea5 to 4a7465a Compare December 14, 2022 18:32
@@ -1071,7 +1071,7 @@ Each :term:`Parameter` directly corresponds to a field in a line of the :abbr:`A
.. _round_robin: https://docs.trafficserver.apache.org/en/9.1.x/admin-guide/files/parent.config.en.html#parent-config-format-round-robin
.. _max_simple_retries: https://docs.trafficserver.apache.org/en/9.1.x/admin-guide/files/parent.config.en.html#parent-config-format-max-simple-retries
.. _max_unavailable_server_retries: https://docs.trafficserver.apache.org/en/9.1.x/admin-guide/files/parent.config.en.html#parent-config-format-max-unavailable-server-retries
.. _parent_retry: https://docs.trafficserver.apache.org/en/9.1.x/admin-guide/files/parent.config.en.html#parent-config-format-parent-retry
.. _parent_retry: https://docs.trafficserver.apache.org/en/9.1.x/admin-guide/files/parent.config.en.html#parent-config-format-parent-retry (deprecated)
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the link anchor, it now takes you to the top of the page (because there's no such anchor) instead of to the description of parent_retry

CHANGELOG.md Outdated
@@ -29,6 +29,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- [#7037](https://github.com/apache/trafficcontrol/pull/7037) *Traffic Router* Uses Traffic Ops API 4.0 by default
- [#7191](https://github.com/apache/trafficcontrol/issues/7191) *tc-health-client* Uses Traffic Ops API 4.0. Also added reload option to systemd service file
- [#4654](https://github.com/apache/trafficcontrol/pull/4654) *Traffic Ops, Traffic Portal* Switched Delivery Service active state to a three-value system, adding a state that will be used to prevent cache servers from deploying DS configuration.
- Switched Delivery Service active state to a three-value system, adding a state that will be used to prevent cache servers from deploying DS configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

This line appears to duplicate the one preceding it.

@ocket8888 ocket8888 merged commit 12e019d into apache:master Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation related to documentation improvement The functionality exists but it could be improved in some way. low impact affects only a small portion of a CDN, and cannot itself break one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants