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

Improved throughput: reduce unnecessary overload on gRPC bidirectional stream by avoiding empty responses #99

Conversation

rafaeling
Copy link
Contributor

@rafaeling rafaeling commented Nov 11, 2024

Databroker was underperforming compared to sdv.databroker.v1 APIs implementation.
gRPC bidirectional stream buffer was being overloaded due to the unnecessary return of empty responses, which caused reduced throughput and buffer congestion.

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 23.52941% with 117 lines in your changes missing coverage. Please review.

Project coverage is 59.27%. Comparing base (e737ae0) to head (5e97dec).
Report is 1 commits behind head on feature/databroker-api-v2.

Files with missing lines Patch % Lines
...tabroker/src/grpc/sdv_databroker_v1/conversions.rs 0.00% 64 Missing ⚠️
databroker/src/grpc/kuksa_val_v2/conversions.rs 25.00% 51 Missing ⚠️
databroker/src/grpc/kuksa_val_v2/val.rs 88.88% 2 Missing ⚠️
Additional details and impacted files
@@                      Coverage Diff                      @@
##           feature/databroker-api-v2      #99      +/-   ##
=============================================================
- Coverage                      59.36%   59.27%   -0.10%     
=============================================================
  Files                             33       33              
  Lines                          16005    16026      +21     
=============================================================
- Hits                            9502     9499       -3     
- Misses                          6503     6527      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rafaeling rafaeling changed the title Reduce unnecessary overload on gRPC bidirectional stream by avoiding empty responses Improved throughput: reduce unnecessary overload on gRPC bidirectional stream by avoiding empty responses Nov 11, 2024
if let Err(err) = response_stream_sender.send(Ok(response)).await
{
debug!("Failed to send response: {}", err);
if let Some(value) = response {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should better clarify in proto files when response shall be sent. Today my interpretation of the text below is that you ALWAYS should get a Response message. We should better clarify with something like "Databroker returns PublishValuesResponse upon error, and nothing upon success"

Current text:

Provider sends PublishValuesRequest -> Databroker returns PublishValuesResponse

Similar for other similar cases

@rafaeling
Copy link
Contributor Author

I close this one and create a new one, branched off the wrong branch. #100

@rafaeling rafaeling closed this Nov 11, 2024
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.

2 participants