-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Skip unknown settings with warnings. #7653
Skip unknown settings with warnings. #7653
Conversation
I prefer to simply pass all settings as strings in new format. This will make code easier. |
5dd4fca
to
c4b49e6
Compare
Done. |
fdd9d59
to
428f93e
Compare
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.
Current behaviour caused us many headaches, but in my opinion this approach is far from ideal. I don't think the server should be the one deciding on silently (from client's perspective) ignore settings.
This might seem like a personal preference, but I'll try and prove my point by going through some examples for which this approach might be fatal.
totals_mode
distributed_product_mode
join_use_nulls
extremes
Suppose these (or similar settings which change query behaviour/result) are introduced in a new version which is not yet fully deployed on a cluster. A user might use them and get wrong results without realising it (in theory we can forbid adding new setting that changes query behaviour and instead add them at query syntax level).
Also, this PR doesn't seem complete: it should probably allow to ignore unknown settings values as well.
A better solution in my opinion would be for the server to expose list of supported settings (optionally, supported values), and let the client decide whether or not these settings are enough to execute the query.
I think that changing way of serialization and have a possibility to ignore unknown settings is good . At the same time the decision "to ignore / not ignore" particular (or all) settings should be controlled by user IMO (from both ends - client or server). So client can send smth like 'ignore_unknown_settings=0' (by default), can be set to 1 during rolling updates in cluster, or even "ignore_unknown_settings=totals_mode,distributed_product_mode" (will be ignored only is server does not support them). Or server admin can decide that he doesn't care if newer clients send something like 'mimic_old_version_behavior=true'. |
I'll try to implement a slightly different solution. The client will send
It seems the client will be able to calculate that |
Does it mean that the decision allowing / forbidding ignorance will be hardcoded? |
…:findStrict(). Rename SettingsCommon.h -> SettingsCollection.h for consistency.
… query is older than a shard.
98a3a80
to
e40c140
Compare
The names and meanings of the settings are hardcoded, so it seems the ignorance flag should be hardcoded too. The developers of new settings will decide whether they can be ignored or not. |
…nings Skip unknown settings with warnings.
…with-warnings" This reverts commit 40cb971.
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category:
Changelog entry:
Allow skipping unknown settings with warnings instead of throwing exceptions.