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
Allow to define volume_priority
in storage_configuration
#58533
Allow to define volume_priority
in storage_configuration
#58533
Conversation
29fdfa7
to
8eddc1a
Compare
Consider std::stable_sort instead of std::sort to avoid unpleasant surprises for users. |
ae5b390
to
8376c71
Compare
The statement in doc "If two or more volumes have the same setting value, order among them is undefined" is not true. To me it is enough to say that
I am a bit worried that higher value actually means lower priority, it may be contrintuitive. |
Already fixed |
Hm ... not pushed yet? |
0e57b00
to
fb42bdb
Compare
A-ha, fixed one line and left another one :) Fixed. |
fb42bdb
to
d2e4520
Compare
This is an automated comment for commit b4e582f with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
5b4a0c2
to
12ac731
Compare
12ac731
to
661fa9c
Compare
f6d6203
to
7a7a6b6
Compare
tests/queries/0_stateless/02961_storage_config_volume_priority.reference
Outdated
Show resolved
Hide resolved
44a778d
to
3929548
Compare
d129c89
to
47e75bd
Compare
|
Could anyone pls take a look at this PR? It's tiny, non-complicated, well-documented and the tests are fine :) cc @alexey-milovidov (as you are the only one from ClickHouse Inc. in this thread 😃 ) |
@@ -870,6 +870,11 @@ Tags: | |||
- `load_balancing` - Policy for disk balancing, `round_robin` or `least_used`. | |||
- `least_used_ttl_ms` - Configure timeout (in milliseconds) for the updating available space on all disks (`0` - update always, `-1` - never update, default is `60000`). Note, if the disk can be used by ClickHouse only and is not subject to a online filesystem resize/shrink you can use `-1`, in all other cases it is not recommended, since eventually it will lead to incorrect space distribution. | |||
- `prefer_not_to_merge` — You should not use this setting. Disables merging of data parts on this volume (this is harmful and leads to performance degradation). When this setting is enabled (don't do it), merging data on this volume is not allowed (which is bad). This allows (but you don't need it) controlling (if you want to control something, you're making a mistake) how ClickHouse works with slow disks (but ClickHouse knows better, so please don't use this setting). | |||
- `volume_priority` — Defines the priority (order) in which volumes are filled. Lower value means higher priority. The parameter values should be natural numbers and collectively cover the range from 1 to N (lowest priority given) without skipping any numbers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be much better if:
- we allow arbitrary unsigned numbers for priorities;
- we allow identical priorities;
- if the priority is identical, the order in the configuration matters;
- omitting the priority value is equivalent to priority zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we allow arbitrary unsigned numbers for priorities
This is about a mismatch of internal representation vs what user specifies in config. E.g. if we specify priorities 1,3,5,7, they would be stored as 1,2,3,4 internally. Moreover, we will have 1,2,3,4 in output of storage_policies
. Wouldn't that be confusing? However, I understand that changing this config may be tough as user will have to reset all priorities. If this internal vs config mismatch is ok, we can do it like this
omitting the priority value is equivalent to priority zero
I think it is a matter of taste. In both ways, it is easy to add a low-priority and high-priority volume.
However, the way it works now we can also easily add a volume in the middle, e.g. 2nd priority volume. With your idea, it would be harder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically these restrictions were introduced in 47e75bd to avoid confusion
and to guarantee that volume_priority in config and in system.storage_policies are the same.
@alexey-milovidov , if you think that it does not make sense, we can return to initial approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
2d97326
to
7c3dfc9
Compare
13b0357
to
e06d1fe
Compare
Investigating fails:
|
@alexey-milovidov updated according to your comment. |
Could you please merge with master? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
The only fails now are: Integration tests (tsan) [3/6]:
|
Allow to manually set volume priorities. Closes #31264.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Allow to define
volume_priority
instorage_configuration
.Documentation entry for user-facing changes