Skip to content

ADR-037: DeliverBlock#3420

Merged
ebuchman merged 9 commits intotendermint:developfrom
danil-lashin:ADR-037
May 5, 2019
Merged

ADR-037: DeliverBlock#3420
ebuchman merged 9 commits intotendermint:developfrom
danil-lashin:ADR-037

Conversation

@danil-lashin
Copy link
Contributor

@danil-lashin danil-lashin commented Mar 13, 2019

ADR for issue #2901

@codecov-io
Copy link

codecov-io commented Mar 13, 2019 •
edited
Loading

Codecov Report

Merging #3420 into develop will increase coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3420      +/-   ##
===========================================
+ Coverage    64.26%   64.32%   +0.05%     
===========================================
  Files          214      214              
  Lines        17513    17493      -20     
===========================================
- Hits         11255    11252       -3     
+ Misses        5316     5305      -11     
+ Partials       942      936       -6
Impacted Files Coverage Δ
libs/db/mem_batch.go 89.47% <0%> (-3.12%) ⬇️
evidence/reactor.go 61.17% <0%> (-3.11%) ⬇️
privval/signer_remote.go 80% <0%> (-2%) ⬇️
blockchain/reactor.go 70.53% <0%> (-0.97%) ⬇️
libs/db/debug_db.go 20.31% <0%> (+0.62%) ⬆️
blockchain/pool.go 80.33% <0%> (+0.67%) ⬆️
rpc/client/httpclient.go 66.51% <0%> (+0.89%) ⬆️
privval/socket_listeners.go 89.65% <0%> (+3.44%) ⬆️
privval/signer_service_endpoint.go 89.09% <0%> (+3.63%) ⬆️
privval/signer_validator_endpoint.go 87.77% <0%> (+12.22%) ⬆️

@melekes
Copy link
Contributor

melekes commented Mar 26, 2019

Thank you for writing this 👍

Do you also want to expand on the upgrade procedure? Will existing applications be forced to implement this method (breaking release)? Or should we check if the application supports DeliverBlock?

Do you want to participate in the Tendermint dev session related to upcoming ABCI changes?

@danil-lashin
Copy link
Contributor Author

danil-lashin commented Mar 28, 2019

Hi @melekes!

Sure, I will and details to this proposal and then change the label to R4R.

I think that it is better to force all applications to implement DeliverBlock method because it will simplify the whole ABCI architecture of Tendermint, speed up communications and simplify crash recovery. It's worth the effort.

@danil-lashin danil-lashin requested a review from xla as a code owner April 12, 2019 14:13
@danil-lashin danil-lashin changed the title [WIP] [ADR] ADR-037: DeliverBlock [WIP] [R4R] ADR-037: DeliverBlock Apr 12, 2019
@danil-lashin danil-lashin changed the title [WIP] [R4R] ADR-037: DeliverBlock [R4R] ADR-037: DeliverBlock Apr 12, 2019
@danil-lashin
Copy link
Contributor Author

danil-lashin commented Apr 12, 2019

Thank you for your patience!
@melekes @xla @ebuchman waiting for your comments.

@xla
Copy link
Contributor

xla commented Apr 12, 2019

@danil-lashin Quick feedback, we merged another ADR in the meantime, so this would be adr038. Sorry for the inconvenience.

@danil-lashin danil-lashin changed the title [R4R] ADR-037: DeliverBlock [R4R] ADR-038: DeliverBlock Apr 12, 2019
@danil-lashin
Copy link
Contributor Author

danil-lashin commented Apr 12, 2019

Not a problem, I've changed the ID.

@xla
Copy link
Contributor

xla commented Apr 12, 2019

@danil-lashin Made a mistake we have pre-assigned ADR numbers in #2313 and yours was indeed 37. sorry for the confusion and noise. :<

@xla xla changed the title [R4R] ADR-038: DeliverBlock ADR-038: DeliverBlock Apr 12, 2019
@danil-lashin
Copy link
Contributor Author

danil-lashin commented Apr 14, 2019

@xla ok! 😅Just let me know the final decision, I will change PR.

@xla
Copy link
Contributor

xla commented Apr 14, 2019

@danil-lashin 37 is final. :>

@xla xla changed the title ADR-038: DeliverBlock ADR-037: DeliverBlock Apr 14, 2019
@melekes
Copy link
Contributor

melekes commented May 1, 2019

For people viewing this PR, we're still discussing whenever we actually need DeliverBlock functionality https://github.com/tendermint/tendermint/issues/2901#issuecomment-477732701

ebuchman
ebuchman reviewed May 5, 2019
View reviewed changes
Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

We can merge this as a proposal but will require further consideration before we decide to move forward with it. Thanks for writing this up.

@ebuchman ebuchman merged commit e2f5471 into tendermint:develop May 5, 2019
@melekes melekes mentioned this pull request May 7, 2019
Closed
36 tasks
@melekes melekes mentioned this pull request May 30, 2019
Closed
44 tasks
brapse pushed a commit to brapse/tendermint that referenced this pull request Jun 5, 2019
@brapse
ADR-037: DeliverBlock (tendermint#3420)
cad1e55 
* ADR 037 Initial draft

* Add link to initial conversation

* Update initial draft

* Fix indentations

* Change negative consequences

* Fix indentations

* Change ADR id

* Add one more positive consequence

* Rollback ADR number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@melekes melekes Awaiting requested review from melekes

@xla xla Awaiting requested review from xla

1 more reviewer

@ebuchman ebuchman ebuchman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danil-lashin @codecov-io @melekes @xla @ebuchman