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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -288,13 +288,27 @@ try | |
std::string path; | ||
|
||
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(); | ||
} | ||
else if (config().has("keeper_server.snapshot_storage_path")) | ||
{ | ||
path = std::filesystem::path(config().getString("keeper_server.snapshot_storage_path")).parent_path(); | ||
} | ||
else | ||
path = std::filesystem::path{KEEPER_DEFAULT_PATH}; | ||
{ | ||
path = KEEPER_DEFAULT_PATH; | ||
} | ||
|
||
std::filesystem::create_directories(path); | ||
|
||
|
@@ -330,6 +344,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 commentThe 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 |
||
global_context->setPath(path); | ||
global_context->setRemoteHostFilter(config()); | ||
|
||
|
@@ -365,7 +380,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 commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
FourLetterCommandFactory::registerCommands(*global_context->getKeeperDispatcher()); | ||
|
||
auto config_getter = [&] () -> const Poco::Util::AbstractConfiguration & | ||
|
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
andkeeper_server.snapshot_storage_path
set, but don't havekeeper_server.storage_path
. But it doesn't make sense to requirekeeper_server.storage_path
in this case, there's no ambiguityThere 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)