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.

Sign up for GitHub

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

Jump to bottom

[Access] Enable grpc compression #4804

Merged
merged 14 commits into from
Oct 17, 2023
Merged

[Access] Enable grpc compression #4804

merged 14 commits into from
Oct 17, 2023

Conversation

UlyanaAndrukhiv
Copy link
Contributor

@UlyanaAndrukhiv UlyanaAndrukhiv commented Oct 6, 2023 •
edited
Loading

#4503

In this pull request:

  • implemented proposed compression algorithms for gRPC (lz4, snappy, deflate) according to docs
  • added compressing requests to ENs (temporary for all compressors)
  • added compressing responses from ENs (temporary for all compressors)
  • added compressing responses from ANs (temporary for all compressors)
  • added synthetic benchmarks which focuses on measuring the gRPC compression to EN for all proposed compressors (gzip, lz4, snappy, deflate)
  • added integration test

peterargue
peterargue reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

great work @UlyanaAndrukhiv! added a few comments, but this looks pretty close

engine/execution/rpc/engine.go Outdated Show resolved Hide resolved
module/grpcserver/server.go Outdated Show resolved Hide resolved
engine/access/rpc/connection/manager.go Outdated Show resolved Hide resolved
engine/common/grpc/compressor/deflate/deflate.go Outdated Show resolved Hide resolved
cmd/access/node_builder/access_node_builder.go Outdated Show resolved Hide resolved
engine/access/rpc/connection/manager.go Outdated Show resolved Hide resolved
engine/common/grpc/compressor/lz4/lz4.go Outdated Show resolved Hide resolved
@UlyanaAndrukhiv UlyanaAndrukhiv marked this pull request as ready for review October 16, 2023 08:30
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (0435707) 55.79% compared to head (e816f39) 55.04%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4804      +/-   ##
==========================================
- Coverage   55.79%   55.04%   -0.76%     
==========================================
  Files         938      707     -231     
  Lines       87067    61842   -25225     
==========================================
- Hits        48580    34040   -14540     
+ Misses      34812    25151    -9661     
+ Partials     3675     2651    -1024
Flag Coverage Δ
unittests 55.04% <ø> (-0.76%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 234 files with indirect coverage changes

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

peterargue
peterargue approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @UlyanaAndrukhiv!

@durkmurder durkmurder added this pull request to the merge queue Oct 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 17, 2023
@durkmurder durkmurder added this pull request to the merge queue Oct 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 17, 2023
@durkmurder durkmurder added this pull request to the merge queue Oct 17, 2023
Merged via the queue into onflow:master with commit 649a038 Oct 17, 2023
36 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@durkmurder durkmurder durkmurder approved these changes

@peterargue peterargue peterargue approved these changes

@ramtinms ramtinms Awaiting requested review from ramtinms ramtinms is a code owner

@jordanschalm jordanschalm Awaiting requested review from jordanschalm jordanschalm is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@UlyanaAndrukhiv @codecov-commenter @durkmurder @peterargue