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

Integrate gRPC queries into BaseApp #6335

Merged
merged 11 commits into from
Jun 4, 2020
Merged

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jun 3, 2020

ref: #5921

This PR:

  • renames GRPCRouter to GRPCQueryRouter which makes more sense because it is just for queries (other non-query gRPC services would be handled separately)
  • adds GRPCQueryRouter to BaseApp and integrates it into the ABCI query handler

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@aaronc aaronc marked this pull request as ready for review June 3, 2020 21:30
@codecov
Copy link

codecov bot commented Jun 3, 2020

Codecov Report

Merging #6335 into master will increase coverage by 0.03%.
The diff coverage is 54.71%.

@@            Coverage Diff             @@
##           master    #6335      +/-   ##
==========================================
+ Coverage   55.61%   55.65%   +0.03%     
==========================================
  Files         448      448              
  Lines       26947    26961      +14     
==========================================
+ Hits        14987    15005      +18     
+ Misses      10886    10879       -7     
- Partials     1074     1077       +3     

Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Thank you @aaronc, just a minor nit.

baseapp/abci.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM

baseapp/abci.go Outdated Show resolved Hide resolved
baseapp/abci.go Outdated Show resolved Hide resolved
baseapp/abci.go Outdated Show resolved Hide resolved
baseapp/abci.go Outdated Show resolved Hide resolved
baseapp/abci.go Outdated Show resolved Hide resolved
baseapp/abci.go Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jun 4, 2020

This pull request introduces 7 alerts when merging abaf39a into b1f483f - view on LGTM.com

new alerts:

  • 5 for Expression has no effect
  • 2 for Unreachable statement

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK :)

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 4, 2020
@mergify mergify bot merged commit c1355d0 into master Jun 4, 2020
@mergify mergify bot deleted the aaronc/5921-grpc-baseapp branch June 4, 2020 18:48
@aaronc aaronc mentioned this pull request Jun 10, 2020
11 tasks
@clevinson clevinson added this to the v0.39 milestone Jun 11, 2020
@aaronc aaronc mentioned this pull request Jul 1, 2020
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:ABCI C:baseapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants