Skip to content
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

CLICKHOUSE-4109 mlock clickhouse #3553

Merged
merged 13 commits into from
Nov 14, 2018
Merged

Conversation

proller
Copy link
Contributor

@proller proller commented Nov 8, 2018

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

@alexey-milovidov
Copy link
Member

Missing linuxCapability.h.

@alexey-milovidov alexey-milovidov merged commit 37a9af5 into ClickHouse:master Nov 14, 2018
/*
#if NDEBUG
memory_amount > 16000000000
? true // Change me to true in future
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misleading comment.

#if defined(__linux__)
if (config().getBool("mlock_executable",
false // TODO: uncomment after tests:
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless code.

}
else
{
LOG_INFO(log, "It looks like the process has no CAP_IPC_LOCK capability, binary mlock will be disabled."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Five spaces - WTF.

" It could happen due to incorrect ClickHouse package installation."
" You could resolve the problem manually with 'sudo setcap cap_ipc_lock=+ep /usr/bin/clickhouse'."
" Note that it will not work on 'nosuid' mounted filesystems.");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excessive newline.

const auto memory_amount = getMemoryAmount();
{ /// After full config loaded
#if defined(__linux__)
if (config().getBool("mlock_executable",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing an entry in config.xml along with its documentation.

alexey-milovidov added a commit that referenced this pull request Nov 14, 2018

bool hasLinuxCapability(decltype(CAP_NET_ADMIN) cap)
{
static bool res = hasLinuxCapabilityImpl(cap);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally wrong.


namespace
{
bool hasLinuxCapabilityImpl(decltype(CAP_NET_ADMIN) cap)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it.


if (!((1 << cap) & response.effective))
return false;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

alexey-milovidov added a commit that referenced this pull request Nov 14, 2018
alexey-milovidov added a commit that referenced this pull request Nov 22, 2018
Fixed error introduced in #3553 and prevent it happening
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants