-
Notifications
You must be signed in to change notification settings - Fork 487
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
[Merged by Bors] - feat: add record counters to spu #2731
Conversation
6958969
to
504b905
Compare
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.
Overall sounds fine. couple of minor point.
But main concern I have is use tokio
crates/fluvio-spu/Cargo.toml
Outdated
@@ -40,7 +40,9 @@ thiserror = "1" | |||
once_cell = "1.5" | |||
sysinfo = "0.26.0" | |||
nix = { version = "0.25"} | |||
|
|||
async-global-executor = { version = "2.3.0", features = ["tokio"] } |
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 need to use tokio?
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.
removed exporting from this pr
&self.topic_metrics.metrics.context, | ||
value, | ||
&[ | ||
opentelemetry::KeyValue::new("direction", "read"), |
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.
Can some of these be made into const? This is not show stopper.
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.
Not sure if that would be good in this case, lets defer discussion to later
let (header, produce_request) = request.get_header_request(); | ||
trace!("Handling ProduceRequest: {:#?}", produce_request); | ||
|
||
let mut topic_results = Vec::with_capacity(produce_request.topics.len()); | ||
for topic_request in produce_request.topics.into_iter() { | ||
let topic_result = handle_produce_topic(&ctx, topic_request).await; | ||
let topic_result = handle_produce_topic(&ctx, topic_request, &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.
Instead of passing as argument, we can add to context. not show stopper
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.
done
f6a5cb1
to
73fa980
Compare
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
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
bors r+ |
Co-authored-by: Nick Cardin <nick@cardin.email>
Pull request successfully merged into master. Build succeeded: |
Co-authored-by: Nick Cardin <nick@cardin.email>
No description provided.