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
Add changeable_without_restart
column to system.server_settings
#58029
Conversation
src/Core/ServerSettings.h
Outdated
#define M_WITH_FLAG_(M, TYPE, NAME, DEFAULT, DESCRIPTION, FLAGS, RUNTIME_RELOAD) \ | ||
M(TYPE, NAME, DEFAULT, DESCRIPTION, FLAGS) \ |
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.
Looks like too many letters, Can we leave just M
and change a few other macros?
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.
@serxa Hi, I'm sorry, I don't quite understand what you mean. Perhaps you are referring to the macro's name being too long?
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.
I was thinking it should be possible to use M(TYPE, NAME, DEFAULT, DESCRIPTION, FLAGS, RUNTIME_RELOAD) directly, but now I see that SERVER_SETTINGS
is passed directly to DECLARE_SETTINGS_TRAITS
which is used in a lot of places, so changing it is not a good idea.
But, yes macro's name still feels too long to me, just because almost everywhere we use M(X, Y, Z) do declare such a list. Let's ask @rschu1ze for his opinion. I was just glad to see this PR :-)
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.
@serxa Ok, The new macro name does sound awkward, and I will make sure to modify it later.
runtime_reload
column to system.server_settings
This is an automated comment for commit 8cd8552 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
|
@@ -28,7 +47,8 @@ NamesAndTypesList StorageSystemServerSettings::getNamesAndTypes() | |||
{"description", std::make_shared<DataTypeString>()}, | |||
{"type", std::make_shared<DataTypeString>()}, | |||
{"is_obsolete", std::make_shared<DataTypeUInt8>()}, | |||
{"is_hot_reloadable", std::make_shared<DataTypeUInt8>()} | |||
{"is_hot_reloadable", std::make_shared<DataTypeUInt8>()}, | |||
{"runtime_reload", std::make_shared<DataTypeEnum8>(getTypeEnumsAndValues())} |
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.
There is no restriction that new columns must be appended, we can insert them at arbitrary positions and even permute the order of existing columns.
Thoughts:
is_obsolete
should be the last columnis_hot_reloadable
andruntime_reload
are related columns and should therefore be adjacent to each other- let's move the two former columns prior to
is_obsolete
.
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.
runtime_reload
isn't a great name (is_hot_reloadable
isn't either but it is too late to change it, I guess).
So if I understand correctly, 'is_hot_reloadableshows *if* a server setting can be changed and
runtime_reload` shows how it can be changed. There is some duplication here. I think the latter is conceptually a full replacement of the former, right?
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.
@rschu1ze I think perhaps the value of "is_hot_reloadable" is problematic. It seems that only those that undergo changes (where the set value is inconsistent with the displayed value) will be shown as true.
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.
ClickHouse/src/Storages/System/StorageSystemServerSettings.cpp
Lines 38 to 61 in 20c0347
std::unordered_map<std::string, std::string> updated = { | |
{"max_server_memory_usage", std::to_string(total_memory_tracker.getHardLimit())}, | |
{"allow_use_jemalloc_memory", std::to_string(total_memory_tracker.getAllowUseJemallocMmemory())}, | |
{"max_table_size_to_drop", std::to_string(context->getMaxTableSizeToDrop())}, | |
{"max_partition_size_to_drop", std::to_string(context->getMaxPartitionSizeToDrop())}, | |
{"max_concurrent_queries", std::to_string(context->getProcessList().getMaxSize())}, | |
{"max_concurrent_insert_queries", std::to_string(context->getProcessList().getMaxInsertQueriesAmount())}, | |
{"max_concurrent_select_queries", std::to_string(context->getProcessList().getMaxSelectQueriesAmount())}, | |
{"background_buffer_flush_schedule_pool_size", std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundBufferFlushSchedulePoolSize))}, | |
{"background_schedule_pool_size", std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundSchedulePoolSize))}, | |
{"background_message_broker_schedule_pool_size", std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundMessageBrokerSchedulePoolSize))}, | |
{"background_distributed_schedule_pool_size", std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundDistributedSchedulePoolSize))} | |
}; | |
if (context->areBackgroundExecutorsInitialized()) | |
{ | |
updated.insert({"background_pool_size", std::to_string(context->getMergeMutateExecutor()->getMaxThreads())}); | |
updated.insert({"background_move_pool_size", std::to_string(context->getMovesExecutor()->getMaxThreads())}); | |
updated.insert({"background_fetches_pool_size", std::to_string(context->getFetchesExecutor()->getMaxThreads())}); | |
updated.insert({"background_common_pool_size", std::to_string(context->getCommonExecutor()->getMaxThreads())}); | |
} |
res_columns[7]->insert((it != updated.end()) ? true : false); |
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.
@rschu1ze Hi, Can I delete "is_hot_reloadable" and change "runtime_reload" to "hot_reload"?
runtime_reload
column to system.server_settings
changeable_without_restart
column to system.server_settings
152ab28
to
e947ed7
Compare
@skyoct I just came back to your PR (after holidays) and checked deeper. Someone added a boolean column The advantage of the new approach is that ServerSettings.h/cpp and StorageSystemServerSettings.cpp cannot easily go out-of-sync (imagine someone adds But to be fair, all of that is based on the fact that the periodic re-loads of the server config only update the server components (e.g. memory tracking) with the new setting values but do not store the settings themselves (so we can't get them directly). If that was the case, your approach would be better. I actually like it conceptually better as we could then use the Besides that, I named the new column |
This may also force developers to write the reloading handler for a newly added settings. |
Fixes #57902
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Replaced undocumented (boolean) column
is_hot_reloadable
in system tablesystem.server_settings
by (Enum8) columnchangeable_without_restart
with possible valuesNo
,Yes
,IncreaseOnly
andDecreaseOnly
. Also documented the column.