Skip to content

Conversation

@masaori335
Copy link
Contributor

@masaori335 masaori335 commented Oct 24, 2025

Revive #11000 for the config project. Diffs from #11000 are

  1. This deprecates storage.config and volume.config, but keeps them for compatibility, so we can merge to master branch.
  2. Rename spans.id to spans.name for clarity
  3. Add volumes.avg_obj_size and volumes.fragment_size support

If this looks good, I'll work on implementation.

Co-authored-by: Alan M. Carroll <amc@apache.org>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Introduces documentation for the new unified storage.yaml cache configuration, and marks the legacy storage.config and volume.config formats as deprecated. It also updates the admin config index and documents per-volume avg_obj_size and fragment_size overrides.

Changes:

  • Add comprehensive storage.yaml documentation including schema (spans, volumes, volumes.spans), allocation rules, and migration/backwards compatibility information.
  • Mark storage.config and volume.config as deprecated in favor of storage.yaml and cross-link them appropriately.
  • Update the admin “Configuration Files” index to include storage.yaml and to reflect the new deprecation status of storage.config and volume.config.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.

File Description
doc/admin-guide/files/storage.yaml.en.rst New, full reference and examples for storage.yaml, covering storage spans, volume definitions (including avg_obj_size and fragment_size), allocation behavior, and compatibility with existing storage.config/volume.config.
doc/admin-guide/files/storage.config.en.rst Adds an .. important:: block to mark storage.config as deprecated in favor of storage.yaml.
doc/admin-guide/files/volume.config.en.rst Adds an .. important:: block to mark volume.config as deprecated in favor of storage.yaml.
doc/admin-guide/files/index.en.rst Registers storage.yaml in the configuration files toctree and updates one-line descriptions for storage.config, storage.yaml, and volume.config to reflect the new deprecations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

size: 134217728

By default a volume uses all spans, therefore a volume uses all of span "store" because there are no other
volumes. It would be equivalent is using the spans explicitly, e.g.
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

In the example description, the phrase "It would be equivalent is using the spans explicitly" is ungrammatical; it should be "equivalent to using the spans explicitly".

Suggested change
volumes. It would be equivalent is using the spans explicitly, e.g.
volumes. It would be equivalent to using the spans explicitly, e.g.

Copilot uses AI. Check for mistakes.
- id: 3

Instead, suppose the physical spans ("disk.1" and "disk.2") should be split across volumes. This can be done by adding volumes
with only defaults, as the phisycal spans will be divided evenly among four volumes (3 - 6), each volume allocated 25% of
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

In the final example description, "phisycal spans" is misspelled and should be "physical spans".

Suggested change
with only defaults, as the phisycal spans will be divided evenly among four volumes (3 - 6), each volume allocated 25% of
with only defaults, as the physical spans will be divided evenly among four volumes (3 - 6), each volume allocated 25% of

Copilot uses AI. Check for mistakes.

:doc:`volume.config.en`
Defines cache space usage by individual protocols.
Defines cache space usage by individual protocols. (Deprecated :file:`storage.yaml`)
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

The volume.config entry description reads "(Deprecated :file:storage.yaml)" which is grammatically unclear and could be misread as deprecating storage.yaml; this should say something like "(Deprecated in favor of :file:storage.yaml)" to match the wording used for storage.config above.

Suggested change
Defines cache space usage by individual protocols. (Deprecated :file:`storage.yaml`)
Defines cache space usage by individual protocols. (Deprecated in favor of :file:`storage.yaml`)

Copilot uses AI. Check for mistakes.

The following basic example shows 128 MB of cache storage in the "/big_dir" directory

.. code-block: yaml
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Several of the later example blocks use the directive syntax .. code-block: yaml (single colon), e.g. on this line; for Sphinx these should be .. code-block:: yaml (double colon) like the earlier example at the top of the file, otherwise they will not render as code blocks.

Copilot uses AI. Check for mistakes.
- id: 1
size: 100%
spans:
- id: store
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

In this YAML example, the span reference under spans: uses - id: store, but earlier in the document the keys for volumes:spans are defined as use and size; this example should use use: store (or otherwise match the documented schema) to avoid confusing readers.

Suggested change
- id: store
- use: store

Copilot uses AI. Check for mistakes.
which will effectively clear most of the cache. This can be problem when drives fail and a system
reboot causes the path names to change.

The :arg:`id` option can be used to create a fixed string that an administrator can use to keep the
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

In the Assignment Table section, the paragraph referring to "The :arg:id option" appears to be copied from storage.config and still mentions an id option that does not exist in storage.yaml, while the corresponding functionality is now provided by the hash_seed key; this should be updated so the text refers to the correct key name.

Suggested change
The :arg:`id` option can be used to create a fixed string that an administrator can use to keep the
The :arg:`hash_seed` option can be used to create a fixed string that an administrator can use to keep the

Copilot uses AI. Check for mistakes.

.. important::

Any change to this files can (and almost always will) invalidate the existing cache in its entirety.
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

Typo in the important note: "Any change to this files can ..." should read "Any change to this file can ..." to be grammatically correct.

Suggested change
Any change to this files can (and almost always will) invalidate the existing cache in its entirety.
Any change to this file can (and almost always will) invalidate the existing cache in its entirety.

Copilot uses AI. Check for mistakes.
Assignment Table
----------------

Each storage element defined in :file:`storage.yaml` is divided in to :term:`stripes <cache stripe>`. The
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

In the Assignment Table section, the phrase "divided in to" should be "divided into" ("Each storage element ... is divided into :term:stripes <cache stripe>") for correct grammar.

Suggested change
Each storage element defined in :file:`storage.yaml` is divided in to :term:`stripes <cache stripe>`. The
Each storage element defined in :file:`storage.yaml` is divided into :term:`stripes <cache stripe>`. The

Copilot uses AI. Check for mistakes.
stripe. By default the path for the storage is used as the base string. This ensures that each
stripe has a unique string for the assignment hash. This does make the assignment table very
sensitive to the path for the storage elements and changing even one can have a cascading effect
which will effectively clear most of the cache. This can be problem when drives fail and a system
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

In the Assignment Table description, the sentence "This can be problem when drives fail" is missing an article; it should be "This can be a problem when drives fail".

Suggested change
which will effectively clear most of the cache. This can be problem when drives fail and a system
which will effectively clear most of the cache. This can be a problem when drives fail and a system

Copilot uses AI. Check for mistakes.
-----------------------

In previous versions of |TS| it was possible to have "exclusive" spans which were used by only one volume. This is
now down by specifying the span in the volume and using a size of "100%". E.g. old configuration like ::
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

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

In the Backwards Compatibility section, "This is now down by specifying the span" appears to be a typo and should read "This is now done by specifying the span".

Suggested change
now down by specifying the span in the volume and using a size of "100%". E.g. old configuration like ::
now done by specifying the span in the volume and using a size of "100%". E.g. old configuration like ::

Copilot uses AI. Check for mistakes.
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.

2 participants