Add number of errors to ignore while choosing replicas#11669
Add number of errors to ignore while choosing replicas#11669akuzm merged 10 commits intoClickHouse:masterfrom
Conversation
azat
force-pushed
the
distributed_replica_error_ignore
branch
from
09f8f72 to
c851407
Compare
Renaming settings is almost never good -- we strive to be backwards compatible where possible. Speaking about the names themselves, "cap" is a saturation limit for the number of errors we track for a replica. It's limited so that the error number decays in a sane time, and the replica is not considered broken forever. I think it's not complementary to your new setting, and they don't form any meaningful range, so giving them complementary names would be misleading. I'd name your new setting "distributed_replica_max_ignored_errors". Although it's kind of hard to tell from these names which limit is which... |
Will remember.
Yeah, when I was writing this I have some similar thoughts, ok - was a bad idea anyway.
Ok, will rename |
azat
force-pushed
the
distributed_replica_error_ignore
branch
2 times, most recently
from
e525fd1 to
7ac46d7
Compare
…eplica_max_ignored_errors) This will allow avoid switching to another replica in case of error (since error can be temporary).
azat
force-pushed
the
distributed_replica_error_ignore
branch
from
7ac46d7 to
9bfda65
Compare
At startup, server loads configuration files. However ConfigReloader does not know about already loaded files (files is empty()), hence it will always reload the configuration just after server starts (+ 2 seconds, reload timeout). And on configuration reload the clusters will be re-created, so some internal stuff will be reseted: - error_count - last_used (round_robing) And if the reload will happen during round_robin test it will start querying from the beginning, so let's issue config reload just after start to avoid reload in the middle of the test execution.
|
I'm not sure how admin is supposed to choose the value of this setting. |
|
I think a better way to balance things would be to track the rate of errors (instead of an absolute value) with a sliding window and allow a small variability in the rate of errors between replicas. The ignore value added here seems to help only in the beginning before the err counter reaches that value and then the behaviour will be exactly the same as it currently is until the counters come back to a values less than this min constant. 2c |
|
@nvartolomei The calculated number of errors is already a sliding "window" with exponential smoothing. |
via |
|
What value will be good: 1 or 10 or 100? |
|
It depends, and should be adjusted only after looking at system.clusters (that has error_count rate) + server logs (for issues around distributed queries). I would say that 100 definitely is not good, but 1-10 may be a good idea. Also rate of queries and time of replica error_count recover (distributed_replica_error_half_life) also should be take into account. (at first glance this can be enough) |
That's generally a good idea if there is a non-trivial amount of refactoring. One way to do it is maintain logically separate changes in separate commits -- it works well with the github interface. For very big PRs, we can even merge some commits earlier, to have less conflicts. It's not very convenient for the developer though -- lots of fiddling with interactive rebase. |
And sometimes even trivial may be a problem (especially for backporting)
Well, keeping separate changes in separate commit is a rule of thumb, but this also does not solves issues with conflicts and so on.
ccache helps at least here :) |
Fixed in: ClickHouse#11669 bd45592 ("Fix test_distributed_load_balancing flaps (due to config reload)")
6 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add number of errors to ignore while choosing replicas (
distributed_replica_error_ignore)Detailed description / Documentation draft:
This will allow avoid switching to another replica in case of error
(since error can be temporary).
Refs: #10564
Details
Previous HEAD's: