Skip to content
This repository has been archived by the owner on Feb 23, 2022. It is now read-only.

abci: note on concurrency #258

Merged
merged 2 commits into from
Feb 26, 2021
Merged

abci: note on concurrency #258

merged 2 commits into from
Feb 26, 2021

Conversation

ebuchman
Copy link
Contributor

@ebuchman ebuchman commented Feb 13, 2021

Per the recent issue in the SDK. This note has been long overdue. Also maybe relevant to #254

Copy link
Contributor

@konnov konnov left a comment

Choose a reason for hiding this comment

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

This is a good warning. I only have one comment on the diff.

@@ -37,7 +57,7 @@ pick up from when it restarts. See information on the Handshake, below.
Application state should only be persisted to disk during `Commit`.
Copy link
Contributor

Choose a reason for hiding this comment

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

As in contrast to what?

### Concurrency

In principle, each of the four ABCI connections operate concurrently with one
another. This means applications need to ensure access to state is
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this tendermint state or application. Would be good to differentiate. These two separate states get conflated often

spec/abci/apps.md Outdated Show resolved Hide resolved
Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the addition.

The way this works seems counterintuitive. If the application handles concurrent reads they dont need to go through tendermint. It seems like applications that route reads through tendermint are not reaching their full potential.

@tac0turtle tac0turtle merged commit 227e526 into master Feb 26, 2021
@tac0turtle tac0turtle deleted the bucky/abci-docs branch February 26, 2021 10:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants