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

add api support for abci streaming to external systems #952

Closed
wants to merge 20 commits into from

Conversation

egaxhaj
Copy link
Contributor

@egaxhaj egaxhaj commented Jul 19, 2022

Description

Adds support for streaming BeginBlocker and EndBlocker request and responses to external systems.

closes: #940


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • 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
  • Review Codecov Report in the comment section below once CI passes

@egaxhaj egaxhaj requested a review from a team as a code owner July 19, 2022 18:37
@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

Merging #952 (7ddd870) into main (77b5e42) will decrease coverage by 0.02%.
The diff coverage is 46.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #952      +/-   ##
==========================================
- Coverage   56.79%   56.76%   -0.03%     
==========================================
  Files         181      182       +1     
  Lines       22273    22333      +60     
==========================================
+ Hits        12649    12678      +29     
- Misses       8651     8677      +26     
- Partials      973      978       +5     
Impacted Files Coverage Δ
app/app.go 84.55% <22.22%> (-2.57%) ⬇️
app/streaming/trace/service/service.go 65.71% <65.71%> (ø)

derekadams
derekadams previously approved these changes Jul 19, 2022
Copy link
Contributor

@derekadams derekadams left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@SpicyLemon SpicyLemon left a comment

Choose a reason for hiding this comment

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

This needs unit/integration tests.

I'm also unsure what the difference between this stuff and the plugins stuff is. An actual use case and some unit tests would probably help with that respect though.

app/app.go Outdated Show resolved Hide resolved
internal/streaming/streaming.go Outdated Show resolved Hide resolved
app/streaming/streaming.go Outdated Show resolved Hide resolved
app/streaming/streaming.go Outdated Show resolved Hide resolved
app/app.go Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
@egaxhaj egaxhaj force-pushed the 940-abci-request-response-streaming branch from 20ca01d to f1b6805 Compare July 20, 2022 00:39
@egaxhaj egaxhaj mentioned this pull request Aug 2, 2022
8 tasks
app/streaming/README.md Outdated Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
Copy link
Contributor

@SpicyLemon SpicyLemon left a comment

Choose a reason for hiding this comment

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

Just a couple more small things.

app/streaming/streaming.go Outdated Show resolved Hide resolved
app/streaming/README.md Outdated Show resolved Hide resolved
egaxhaj and others added 2 commits August 3, 2022 14:09
Co-authored-by: Daniel Wedul <github@wedul.com>
@arnabmitra arnabmitra self-requested a review August 9, 2022 16:18
Copy link
Contributor

@arnabmitra arnabmitra left a comment

Choose a reason for hiding this comment

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

i think this PR looks good to me, the "omitted" use case i am not sure i understand but it being controlled by the flag makes it sound code wise..

@egaxhaj
Copy link
Contributor Author

egaxhaj commented Aug 23, 2022

closing this for now in favor of a grpc approach discussed here. Will keep the repo open for reference.

@egaxhaj egaxhaj closed this Aug 23, 2022
@iramiller iramiller deleted the 940-abci-request-response-streaming branch May 16, 2023 16:58
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.

ABCI Request Response Streaming
6 participants