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

3.0 configuration #3825

Merged
merged 5 commits into from
Nov 13, 2023
Merged

3.0 configuration #3825

merged 5 commits into from
Nov 13, 2023

Conversation

andreyaksenov
Copy link
Contributor

@andreyaksenov andreyaksenov commented Oct 31, 2023

@andreyaksenov andreyaksenov linked an issue Oct 31, 2023 that may be closed by this pull request
@andreyaksenov andreyaksenov force-pushed the 3.0-config-etcd branch 13 times, most recently from 05caf81 to 02ea158 Compare November 1, 2023 11:59
@andreyaksenov andreyaksenov changed the base branch from 3.0-config to 3.0 November 3, 2023 07:04
@andreyaksenov andreyaksenov changed the base branch from 3.0 to 3.0-config November 3, 2023 07:05
@andreyaksenov andreyaksenov changed the base branch from 3.0-config to 3.0 November 3, 2023 08:07
@andreyaksenov andreyaksenov changed the base branch from 3.0 to 3.0-config November 3, 2023 08:08
@andreyaksenov andreyaksenov changed the base branch from 3.0-config to 3.0 November 3, 2023 08:08
@andreyaksenov andreyaksenov force-pushed the 3.0-config-etcd branch 2 times, most recently from e906c23 to 6107111 Compare November 3, 2023 08:10
@andreyaksenov andreyaksenov changed the title 3.0 etcd config 3.0 configuration Nov 3, 2023
Copy link
Contributor

@p7nov p7nov left a comment

Choose a reason for hiding this comment

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

Some comments from my side.

Configuration
=============

There are two approaches to configuring Tarantool:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer having an intro: a couple of sentences about what is Tarantool configuration. What's its purpose? Can I just run Tarantool without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, will add


* *Since version 3.0*: In a YAML file.

In a YAML file, you can provide the full cluster topology and specify all configuration options.
Copy link
Contributor

Choose a reason for hiding this comment

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

Repeating In a YAML file. Let's rephase.

* *Since version 3.0*: In a YAML file.

In a YAML file, you can provide the full cluster topology and specify all configuration options.
You can also use :ref:`etcd <configuration_etcd_overview>` to store configuration data in one reliable place.
Copy link
Contributor

Choose a reason for hiding this comment

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

This step from YAML to etcd is surprising. If I understand correctly, etcd doesn't store YAML data. It stores the same declarative config, but has nothing to do with YAML format.
Maybe rename this list item to Declarative configuration and say that it can be in YAML or in etcd?

Copy link
Contributor Author

@andreyaksenov andreyaksenov Nov 10, 2023

Choose a reason for hiding this comment

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

etcd doesn't store YAML data

etcd stores configuration data using YAML. So, it is not a local YAML file but YAML data stored under the specified key on an etcd server. Maybe, worth changing to more general In a YAML format, smth like this:

YAML configuration allows you to provide the full cluster topology and specify all configuration options.
You can use local configuration in a YAML file for each instance or store configuration data in one reliable place using :ref:etcd <configuration_etcd_overview>.

* *In version 2.11 and earlier*: :ref:`In code <configuration_code>` using the ``box.cfg`` API.

In this case, configuration is provided in a Lua initialization script.
Starting with the 3.0 version, configuring Tarantool in code is considered a legacy approach.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe convert to an admonition for more visibility?

Configuration overview
----------------------

A YAML configuration file describes the full topology of a Tarantool cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. It's not always YAML file. To me, a configuration and its format look like separate entities.
  2. It's not only topology. This may be important in the introductory paragraphs (readers don't know the details yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Changed to:

YAML configuration describes the full topology of a Tarantool cluster.

  1. Added some information to the intro.

- ``replicaset_name``
- ``group_name``

To reference such variables in a configuration file, use double curly braces.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To reference such variables in a configuration file, use double curly braces.
To reference these variables in a configuration file, enclose them in double curly braces with whitespaces.

Seems more accurate

* Variables whose names start with ``TT_`` are used to substitute parameters specified in a configuration file.
This means that these variables have a higher :ref:`priority <configuration_precedence>` than the options specified in a configuration file.

* Variables whose names start with ``TT_`` and end with ``_DEFAULT`` are used to specify default values for parameters missing in a configuration file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Schematic variable name with placeholder can help shorten the description:

two sets .. of variables:
- TT_<CONFIG_PARAMETER>. These variables are used ...
- TT_<CONFIG_PARAMETER>_DEFAULT. These variables are used...

$ export TT_LOG_LEVEL=3

* (*Array*) The examples below show how to set the ``TT_SHARDING_ROLES`` variable that accepts an array value.
Arrays can be passed in a *simple* ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure unfinished sentence with ellipsis is good here, especially with italic. I couldn't understand right away.
in a simple or JSON format: <two examples>.


Below are a few examples that show how to set environment variables of different types, like *string*, *number*, *array*, or *map*:

* (*String*) In the example below, ``TT_IPROTO_LISTEN`` is used to specify a :ref:`listening host and port <configuration_options_connection>` values:
Copy link
Contributor

Choose a reason for hiding this comment

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

Punctuation in this list is confusing me. I don't get these brackets and italic. Why not just

- String. In the example ...
- Number. In this example...
-  Array. ...


.. _configuration_command_options:

Command-line options
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely not the place for tarantool binary options reference. Need to find place somewhere in reference or future Tools section and link to it.

Copy link
Contributor

@p7nov p7nov left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple more suggestions

Configuration
=============

Tarantool provides the ability to configure the full topology of a cluster and set parameters specific for concrete instances, like connection settings, memory used to store data, logging, and snapshot settings.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Tarantool provides the ability to configure the full topology of a cluster and set parameters specific for concrete instances, like connection settings, memory used to store data, logging, and snapshot settings.
Tarantool provides the ability to configure the full topology of a cluster and set parameters specific for concrete instances, such as connection settings, memory used to store data, logging, and snapshot settings.

"like" == comparison, similar thing
"such as" == example, items of the set you describe


There are two approaches to configuring Tarantool:

* *Since version 3.0*: In a YAML format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* *Since version 3.0*: In a YAML format.
* *Since version 3.0*: In the YAML format.

It stores data or might act as a router for handling CRUD requests in a :ref:`sharded <sharding>` cluster.
- ``replicasets``

A *replica set* is a pack of instances that operate on copies of the same databases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A *replica set* is a pack of instances that operate on copies of the same databases.
A *replica set* is a pack of instances that operate on same data chunks.

"copies of the same database" may be confusing, because the term "database" can refer to everything stored in the cluster (all buckets of all spaces is one whole database).
Just an suggestion, the wording may be different.

Copy link
Contributor Author

@andreyaksenov andreyaksenov Nov 13, 2023

Choose a reason for hiding this comment

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

I'd use data set here. To me, data chunk sounds like a small piece of data passed over a network.

Access control
~~~~~~~~~~~~~~

The ``credentials`` section allows you to grant the specified privileges to users.
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be helpful to say here that this config creates the users. It's not obvious. I thought that perhaps I should create them somehow and then run this config to grant roles.

- http://localhost:2379
prefix: /example
username: testuser
password: foobar
Copy link
Contributor

Choose a reason for hiding this comment

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

}
print('Starting ', arg[1])

and suppose the environment variable LISTEN_URI contains 3301,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and suppose the environment variable LISTEN_URI contains 3301,
and suppose the environment variable ``LISTEN_URI`` contains 3301,


.. box_cfg_legacy_note_end

This topic covers specifics of configuring Tarantool in code using the ``box.cfg`` API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This topic covers specifics of configuring Tarantool in code using the ``box.cfg`` API.
This topic covers the specifics of configuring Tarantool in code using the ``box.cfg`` API.


export TT_LISTEN="localhost:3301?param1=value1&param2=value2"

An empty variable (``TT_LISTEN=``) has the same effect as an unset one meaning that the corresponding configuration parameter won't be set when calling ``box.cfg{}``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
An empty variable (``TT_LISTEN=``) has the same effect as an unset one meaning that the corresponding configuration parameter won't be set when calling ``box.cfg{}``.
An empty variable (``TT_LISTEN=``) has the same effect as an unset one, meaning that the corresponding configuration parameter won't be set when calling ``box.cfg{}``.

Comment on lines 79 to 80
* ``TT_LISTEN`` -- corresponds to the ``box.cfg.listen`` option.
* ``TT_MEMTX_DIR`` -- corresponds to the ``box.cfg.memtx_dir`` option.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ``TT_LISTEN`` -- corresponds to the ``box.cfg.listen`` option.
* ``TT_MEMTX_DIR`` -- corresponds to the ``box.cfg.memtx_dir`` option.
* ``TT_LISTEN`` -- corresponds to the ``box.cfg.listen`` option.
* ``TT_MEMTX_DIR`` -- corresponds to the ``box.cfg.memtx_dir`` option.

I would add the links to these options


Formally, the URI
syntax is ``[host:]port`` or ``[username:password@]host:port``.
If a host is omitted, then "0.0.0.0" or "[::]" is assumed
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If a host is omitted, then "0.0.0.0" or "[::]" is assumed
If a host is omitted, then "0.0.0.0" or "[::]" is assumed,

Comment on lines 211 to 212
where a URI is expected, for example, "unix/:/tmp/unix_domain_socket.sock" or
simply "/tmp/unix_domain_socket.sock".
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
where a URI is expected, for example, "unix/:/tmp/unix_domain_socket.sock" or
simply "/tmp/unix_domain_socket.sock".
where a URI is expected, for example, ``"unix/:/tmp/unix_domain_socket.sock"`` or
simply ``"/tmp/unix_domain_socket.sock"``.

It's a bit difficult to read these sockets (and find them in text) without any code formatting.

.. literalinclude:: /code_snippets/snippets/config/instances.enabled/etcd/config.yaml
:language: yaml
:dedent:

Copy link
Contributor

Choose a reason for hiding this comment

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

If this port (2379) is default for etcd, it might be useful to mention that.

Comment on lines 44 to 45
In this example, the following options are configured in addition to an etcd endpoint and key prefix:

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In this example, the following options are configured in addition to an etcd endpoint and key prefix:

Repeats the previous paragraphs

@andreyaksenov andreyaksenov merged commit f1277cb into 3.0 Nov 13, 2023
1 check passed
@andreyaksenov andreyaksenov deleted the 3.0-config-etcd branch November 13, 2023 10:17
andreyaksenov added a commit that referenced this pull request Nov 21, 2023
Document a new declarative configuration approach: local YAML file and etcd config
andreyaksenov added a commit that referenced this pull request Dec 6, 2023
Document a new declarative configuration approach: local YAML file and etcd config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment