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
Use the same default paths for clickhouse_keeper (symlink) as for clickhouse_keeper (executable) #52861
Use the same default paths for clickhouse_keeper (symlink) as for clickhouse_keeper (executable) #52861
Conversation
This is an automated comment for commit f55fe91 with description of existing statuses. It's updated for the latest CI running
|
@@ -330,6 +330,7 @@ try | |||
auto global_context = Context::createGlobal(shared_context.get()); | |||
|
|||
global_context->makeGlobalContext(); | |||
global_context->setApplicationType(Context::ApplicationType::KEEPER); |
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.
The main fix is here because Context::initializeKeeperDispatcher() checks the application type and then passes is_standalone_app = (application_type == ApplicationType::KEEPER)
to KeeperContext
which uses it to calculate the default paths.
@@ -365,7 +366,7 @@ try | |||
} | |||
|
|||
/// Initialize keeper RAFT. Do nothing if no keeper_server in config. | |||
global_context->initializeKeeperDispatcher(/* start_async = */ true); | |||
global_context->initializeKeeperDispatcher(/* start_async = */ 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.
The Keeper
class can be used either by clickhouse-keeper
executable or by clickhouse-keeper
symlink. It's not used by clickhouse-server
, so asynchronous starting is not required here. See also the assert in Context::initializeKeeperDispatcher()
.
a8ef34b
to
f813be1
Compare
I'm a little bit concerned about changing the default storage path. If someone runs Keeper with default configs and then updates to the newest version, they will probably see it as a data loss (if they don't know about these changes). |
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 good (except for my concern above)
…ckhouse_keeper (executable), i.e. "/var/lib/clickhouse-keeper/.."
…figuration ('path' must be reserved for clickhouse-server).
d7665f3
to
ca71590
Compare
You're right, that can be not expected. I decided to show an error message if the setting |
ca71590
to
f55fe91
Compare
if (config().has("keeper_server.storage_path")) | ||
{ | ||
path = config().getString("keeper_server.storage_path"); | ||
} | ||
else if (std::filesystem::is_directory(std::filesystem::path{config().getString("path", DBMS_DEFAULT_PATH)} / "coordination")) | ||
{ | ||
throw Exception(ErrorCodes::NO_ELEMENTS_IN_CONFIG, | ||
"By default 'keeper.storage_path' could be assigned to {}, but the directory {} already exists. Please specify 'keeper.storage_path' in the keeper configuration explicitly", | ||
KEEPER_DEFAULT_PATH, String{std::filesystem::path{config().getString("path", DBMS_DEFAULT_PATH)} / "coordination"}); | ||
} | ||
else if (config().has("keeper_server.log_storage_path")) | ||
{ | ||
path = std::filesystem::path(config().getString("keeper_server.log_storage_path")).parent_path(); |
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.
The order of checks is weird. It will throw the exception if we have keeper_server.log_storage_path
and keeper_server.snapshot_storage_path
set, but don't have keeper_server.storage_path
. But it doesn't make sense to require keeper_server.storage_path
in this case, there's no ambiguity
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.
@pufit, would you like to make a follow-up PR with a fix? (Vitaly is on vacation right now)
Changelog category:
Changelog entry:
Use the same default paths for
clickhouse_keeper
(symlink) as forclickhouse_keeper
(executable).Before this PR the default paths were different:
After this PR the default paths for
clickhouse_keeper
(executable) andclickhouse_keeper
(symlink) will be the same:This PR allows to run integration tests with keeper built even without
-DBUILD_STANDALONE_KEEPER=1