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 #100

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.

If we accept this change, Provider will fire and forget since Databroker will not send back any ACK message. Could it overload the gRPC buffer?

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 59.36%. Comparing base (e737ae0) to head (40f071c).
Report is 2 commits behind head on feature/databroker-api-v2.

Files with missing lines Patch % Lines
databroker/src/grpc/kuksa_val_v2/val.rs 75.00% 2 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           feature/databroker-api-v2     #100   +/-   ##
==========================================================
  Coverage                      59.36%   59.36%           
==========================================================
  Files                             33       33           
  Lines                          16005    15998    -7     
==========================================================
- Hits                            9502     9498    -4     
+ Misses                          6503     6500    -3     

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

@@ -793,17 +794,8 @@ async fn publish_values(

// TODO check if provider is allowed to update the entries for the provided signals?
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticed the TODO - Is it so that we currently do not check token, even if broker is configured to require token?
If so, something we better should fix before release - not necessarily part of this PR?

Copy link
Contributor

@erikbosch erikbosch Nov 13, 2024

Choose a reason for hiding this comment

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

But we have let broker = broker.authorized_access(&permissions); earlier, so it is "broker" that does the check on the line below. Could you confirm @argerus

If so just remove the TODO above.

@argerus argerus merged commit de2730e into eclipse-kuksa:feature/databroker-api-v2 Nov 13, 2024
23 checks passed
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