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.

Sign up for GitHub

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

Jump to bottom

Allow to write ClickHouse text logs into system table. #6103

Merged
merged 29 commits into from
Aug 4, 2019
Merged

Allow to write ClickHouse text logs into system table. #6103

merged 29 commits into from
Aug 4, 2019

Conversation

nikitamikhaylov
Copy link
Member

@nikitamikhaylov nikitamikhaylov commented Jul 22, 2019 •
edited by KochetovNicolai
Loading

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

Category (leave one):

  • New Feature

Short description (up to few sentences):
Now text logs saved in system table.

To enable, add

    <text_log>
        <database>system</database>
        <table>text_log</table>
        <flush_interval_milliseconds>7500</flush_interval_milliseconds>
    </text_log>

in config.xml.

#6037

@nikitamikhaylov
Copy link
Member Author

Please add some lines to server config to create system.text_log table.

alexey-milovidov
alexey-milovidov reviewed Jul 22, 2019
View reviewed changes
dbms/src/Interpreters/TextLog.cpp Outdated
auto priority_datatype = std::make_shared<DataTypeEnum8>(
DataTypeEnum8::Values
{
{"FATAL", static_cast<Int8>(Message::PRIO_FATAL)},
Copy link
Member

Choose a reason for hiding this comment

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

I prefer Fatal not FATAL, like in text log itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@nikitamikhaylov nikitamikhaylov changed the title [WIP] Allow to write ClickHouse text logs into system table. Allow to write ClickHouse text logs into system table. Jul 23, 2019
@alexey-milovidov alexey-milovidov mentioned this pull request Jul 25, 2019
Closed
@alexey-milovidov
Copy link
Member

alexey-milovidov commented Jul 25, 2019 •
edited
Loading

Please pull changes from my branch: #6164

TODO:

  • source file and line don't work at all; you have to add this information via LOG_ macros;
  • it logs only events related to queries and merges (where ThreadGroup exists). But we need all logs from some point after startup to some point before shutdown. Maybe better to pull TextLog out of Context and access it via Singleton.
  • the text doesn't work.

@nikitamikhaylov
Copy link
Member Author

What are the changes from your branch aimed at? I’ve already solved the problem with text_log setting in server’s config.

@alexey-milovidov
Copy link
Member

e5df077

  • documentation for ClickHouse DBA in default server config;

2a9d606

  • enable text log when you run clickhouse-server from source tree (intended for developers);

45c5dd8

  • style fix + remove redundand temporary objects.

@alexey-milovidov
Copy link
Member

Also please add function column (from __PRETTY_FUNCTION__ macro in a location of LOG_*)

@alexey-milovidov alexey-milovidov merged commit a059921 into ClickHouse:master Aug 4, 2019
alexey-milovidov
alexey-milovidov reviewed Aug 4, 2019
View reviewed changes
dbms/src/Interpreters/Context.cpp
{
auto lock = getLock();

if (!shared->system_logs)
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is written for SYSTEM FLUSH LOGS functionality. Because it requires all system logs as Context’s field.

Copy link
Member

Choose a reason for hiding this comment

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

Null pointer dereference.

Copy link
Member Author

Choose a reason for hiding this comment

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

ohh, yes..

@nikitamikhaylov
Copy link
Member Author

If the code is really awful, maybe I should rewrite It with separate complex singleton?

@nikitamikhaylov nikitamikhaylov added the pr-feature Pull request with new product feature label Sep 19, 2019
azat
azat reviewed Jan 24, 2020
View reviewed changes
dbms/tests/queries/0_stateless/00974_text_log_table_not_empty.sh

${CLICKHOUSE_CLIENT} --query="SELECT 6103"

for (( i=1; i <= 50; i++ ))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What for 50 retries here with 0.1 delay, if there is system flush logs? AFAICS system flush logs is sync

Copy link
Member Author

Choose a reason for hiding this comment

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

Because there are too many buffers. Do you have any problems with this test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because there are too many buffers.

Sorry don't get it, what buffers?

Do you have any problems with this test?

Not really (in case of failure it can take pretty long, but that was due to my changes so not a problem)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azat azat azat left review comments

@alexey-milovidov alexey-milovidov alexey-milovidov left review comments

Assignees
No one assigned
Labels
pr-feature Pull request with new product feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nikitamikhaylov @alexey-milovidov @azat