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

Forward profile events to client #28364

Merged
merged 19 commits into from
Oct 12, 2021
Merged

Forward profile events to client #28364

merged 19 commits into from
Oct 12, 2021

Conversation

novikd
Copy link
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 marked this pull request as ready for review September 19, 2021 11:56
@novikd
Copy link
Member Author

novikd commented Sep 27, 2021

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2021

Command update: success

Branch has been successfully updated

@CLAassistant
Copy link

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
Member

What will I see in case of distributed queries?

@alexey-milovidov
Copy link
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
Member

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

@novikd
Copy link
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
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
Collaborator

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
Member

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

@alexey-milovidov
Copy link
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