Skip to content

Forward profile events to client#28364

Merged
nikitamikhaylov merged 19 commits intomasterfrom
profile-events-wip
Oct 12, 2021
Merged

Forward profile events to client#28364
nikitamikhaylov merged 19 commits intomasterfrom
profile-events-wip

Conversation

@novikd
Copy link
Copy Markdown
Member

@novikd novikd commented Aug 30, 2021

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

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Send profile events from server to client. New packet type ProfileEvents was introduced. Closes #26177.

@robot-clickhouse robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Aug 30, 2021
@novikd novikd force-pushed the profile-events-wip branch from 6aa07d4 to 662a59e Compare September 7, 2021 14:45
@novikd novikd marked this pull request as ready for review September 19, 2021 11:56
@novikd
Copy link
Copy Markdown
Member Author

novikd commented Sep 27, 2021

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 27, 2021

Command update: success

Branch has been successfully updated

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Sep 28, 2021

CLA assistant check
All committers have signed the CLA.

@nikitamikhaylov nikitamikhaylov self-assigned this Sep 28, 2021
@novikd novikd force-pushed the profile-events-wip branch 2 times, most recently from 5237752 to c2ef62f Compare October 4, 2021 22:04
@nikitamikhaylov
Copy link
Copy Markdown
Member

What will I see in case of distributed queries?

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Oct 6, 2021

@nikitamikhaylov Events for distributed queries are just forwarded (not merged anyhow) so the events from different servers can be distinguished on the client to show various summaries like: maximum memory usage across all servers, total memory usage summed across all servers, multiple individual progress bars, etc...

@alexey-milovidov
Copy link
Copy Markdown
Member

This is how it's supposed to work, I did not read the PR though :)

@novikd
Copy link
Copy Markdown
Member Author

novikd commented Oct 6, 2021

All profile events are supposed to be forwarded to the client (You can see that in RemoteQueryExecutor). The client handles all the data. After this change it will compute number of threads, cores and memory used on all the hosts.

std::vector<ProfileEventsSnapshot> snapshots;
ProfileEventsSnapshot group_snapshot;
{
std::lock_guard guard(thread_group->mutex);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

snapshots.reserve() after we acquire a lock?

@azat
Copy link
Copy Markdown
Member

azat commented Oct 16, 2021

@novikd Maybe ProfileEvents packets should be sent only if they were requested, like send_logs_level (and enable it automatically on client if some options was enabled)?
This will also fix #30134

P.S. I've also verified that it does not decrease throughput of a simple select 1 query (i.e. clickhouse-benchmark --query 'select 1' -i10000)

@alexey-milovidov
Copy link
Copy Markdown
Member

@azat Better to avoid complications.
I see it can be needed only for tests.

@alexey-milovidov
Copy link
Copy Markdown
Member

alexey-milovidov commented Nov 5, 2021

@nikitamikhaylov The whole implementation is wrong.

increment values should send increments, not accumulated values.

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

Labels

pr-feature Pull request with new product feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forward profile events to client

6 participants