-
Notifications
You must be signed in to change notification settings - Fork 14
Profile events for task distribution in ObjectStorageCluster requests #1172
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
Profile events for task distribution in ObjectStorageCluster requests #1172
Conversation
src/Common/ProfileEvents.cpp
Outdated
| M(ParquetMetaDataCacheMisses, "Number of times the read from filesystem cache miss the cache.", ValueType::Number) \ | ||
| \ | ||
| M(ObjectStorageClusterSentToMatchedReplica, "Number of tasks in ObjectStorageCluster request sent on matched replica.", ValueType::Number) \ | ||
| M(ObjectStorageClusterSentToNonMatchedReplica, "Number of tasks in ObjectStorageCluster request sent on non-matched replica.", ValueType::Number) \ |
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.
sent to on ...
| /// Now this sleep is on executor node in worker thread | ||
| /// Does not block query initiator | ||
| ProfileEvents::increment(ProfileEvents::ObjectStorageClusterWaitingMicroseconds, retry_after_us.value()); | ||
| sleepForMicroseconds(std::min(Poco::Timestamp::TimeDiff(100000ul), retry_after_us.value())); |
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.
Why do we ignore 100000ul for profile event?
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.
Ups
| ); | ||
|
|
||
| ProfileEvents::increment(ProfileEvents::ObjectStorageClusterSentToNonMatchedReplica); | ||
| return next_file; |
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.
Are we sure that replica is not matched here?
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.
Yes, here is only when no matched files anymore.
ilejn
left a comment
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.
LGTM.
Just minor questions.
|
Test failures worth analyzing. |
…cluster_profile_events
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| namespace ProfileEvents | ||
| { | ||
| extern const Event ObjectStorageClusterSentToMatchedReplica; | ||
| extern const Event ObjectStorageClusterSentToNonMatchedReplica; | ||
| }; |
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.
Add ProfileEvents header for new counters
This file now declares ProfileEvents::ObjectStorageClusterSentToMatchedReplica/...NonMatchedReplica and calls ProfileEvents::increment, but it never includes <Common/ProfileEvents.h>, so Event and increment are undefined in this translation unit. As written the code will not compile after this commit; the missing header (or forward declaration of increment plus the Event typedef) needs to be added.
Useful? React with 👍 / 👎.
…cluster_profile_events
…orage_cluster_profile_events Profile events for task distribution in ObjectStorageCluster requests
|
Verification test https://github.com/Altinity/clickhouse-regression/blob/main/swarms/tests/object_storage_cluster_profile_events.py The test creates a cluster with multiple nodes, sets up an overload scenario with one slow/overloaded node, |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Profile events for task distribution in ObjectStorageCluster requests
Documentation entry for user-facing changes
New profile events:
lock_object_storage_task_distribution_mstimeout (calculated on replicas)CI/CD Options
Exclude tests:
Regression jobs to run: