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

[Merged by Bors] - feat: add record counters to spu #2731

Closed
wants to merge 4 commits into from

Conversation

nacardin
Copy link
Contributor

@nacardin nacardin commented Oct 20, 2022

No description provided.

@nacardin nacardin marked this pull request as ready for review October 21, 2022 17:38
@nacardin nacardin requested review from galibey and sehz October 21, 2022 18:30
Copy link
Contributor

@sehz sehz left a 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

@@ -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"] }
Copy link
Contributor

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?

Copy link
Contributor Author

@nacardin nacardin Oct 21, 2022

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"),
Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@galibey galibey left a comment

Choose a reason for hiding this comment

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

LGTM

@nacardin nacardin requested a review from sehz October 21, 2022 21:23
Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

LGTM

@sehz sehz added this to the 0.10.0 milestone Oct 21, 2022
@sehz
Copy link
Contributor

sehz commented Oct 21, 2022

bors r+

bors bot pushed a commit that referenced this pull request Oct 21, 2022
Co-authored-by: Nick Cardin <nick@cardin.email>
@bors
Copy link

bors bot commented Oct 21, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: add record counters to spu [Merged by Bors] - feat: add record counters to spu Oct 21, 2022
@bors bors bot closed this Oct 21, 2022
davidbeesley pushed a commit to davidbeesley/fluvio that referenced this pull request Oct 31, 2022
Co-authored-by: Nick Cardin <nick@cardin.email>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants