-
Notifications
You must be signed in to change notification settings - Fork 316
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
chore: carry metrics in flight metadata from datanode to frontend #3113
chore: carry metrics in flight metadata from datanode to frontend #3113
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3113 +/- ##
==========================================
- Coverage 85.63% 85.13% -0.51%
==========================================
Files 829 829
Lines 135773 135866 +93
==========================================
- Hits 116273 115668 -605
- Misses 19500 20198 +698 |
cc @NiwakaDev you might be interested in this |
Why not put the metrics in https://arrow.apache.org/docs/format/Flight.html#protocol-buffer-definitions |
FlightMessage::Metrics(s) => {
let metadata = FlightMetadata {
affected_rows: None,
metrics: Some(Metrics {
metrics: s.as_bytes().to_vec(),
}),
}
.encode_to_vec();
FlightData {
flight_descriptor: None,
data_header: build_none_flight_msg().into(),
app_metadata: metadata.into(),
data_body: ProstBytes::default(),
}
} It's now put in |
@shuiyisong Are the metrics mainly for the Frontend to deal with RCUs? If so, is it possible to break the whole metrics into each recordbatch's metadata, instead of put them at end of a recordbatch stream? This comes in two favor:
|
No. This feature aims to provide a way to pass execution metrics through a distributed plan tree. A simple extension over
As said above, this is derived from
I doubt if this is viable. Do we or Can we have such a real-time CU administration system? |
4d488b6
to
d7c2a44
Compare
if let Some(m) = recordbatches.metrics() { | ||
let metrics = FlightMessage::Metrics(m); | ||
let _ = tx.send(Ok(metrics)).await; | ||
} |
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.
cc @waynexia
If transmission of metrics fails, I guess that it might be better to inform frontend
about it. This might be because frontend
continue to wait for metrics corresponding to each distributed query when executing DISTRIBUTED ANALYZE PLAN
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.
What do you mean by "inform"? The current implementation puts metrics at the end of each stream. This is because the metric is only available when a corresponding plan is finished.
For distributed analyze scenario, what do you think of discarding data and making the metric the first and only result?
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
Co-authored-by: Ruihang Xia <[email protected]>
Co-authored-by: Ruihang Xia <[email protected]>
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
I hereby agree to the terms of the GreptimeDB CLA
What's changed and what's your intention?
This pr mainly supports carrying metrics from datanode to frontend using metrics in flight metadata.
We use
RecordBatchMetrics
in json format for now. A better form is expected after #2374discussion: is it possible to reduce the number of stream wrapper?
Checklist
Refer to a related PR or issue link (optional)