abci/client: simplify client interface by creachadair · Pull Request #7607 · tendermint/tendermint

creachadair · 2022-01-15T07:50:29Z

As we discussed at the ABCI++ meeting, the rollout in v0.36 is an ideal time to clean up some infelicities in the original implementation. This is a first step toward that cleanup. We will still have to address Flush, CommitTx, and DeliverTx separately, but getting the easy cases out of the way will help keep those diffs smaller.

N.B.: The diff in grpc_client.go is large, but less complicated than it looks. It folds the sync/async versions back together, strips out the unnecessary support code, and updates the names. The rename makes the diff look hairier than it should be.

This change has two main effects:

Remove most of the Async methods from the abci.Client interface.
Remaining are FlushAsync, CommitTxAsync, and DeliverTxAsync.
Rename the synchronous methods to remove the "Sync" suffix.

The rest of the change is updating the implementations, subsets, and mocks of
the interface, along with the call sites that point to them.

TODO ~~before~~ after merge:

Update changelog (Update pending changelog from #7607. #7666).

tac0turtle

love to see this being cleanup. Before merging might want to do a sanity check with @ebuchman, I believe he may know why this exists.

Im for this change and in the sdk we only use the local client, so not sure where all this came in

cmwaters

LGTM pending a check from @ebuchman or @milosevic

cmwaters · 2022-01-18T11:07:50Z

abci/client/client.go

@@ -33,35 +33,24 @@ type Client interface {

 	// Asynchronous requests
 	FlushAsync(context.Context) (*ReqRes, error)


Why do we still need this?

We might not. It's called in the mempool and the proxy app. It might be we could get rid of those, or swap them to synchronous, but I didn't want to expand this change with functional changes.

Yeah makes sense to tackle this as a different issue/PR

cmwaters · 2022-01-18T11:12:32Z

abci/client/client.go

-	EchoAsync(ctx context.Context, msg string) (*ReqRes, error)
-	InfoAsync(context.Context, types.RequestInfo) (*ReqRes, error)
 	DeliverTxAsync(context.Context, types.RequestDeliverTx) (*ReqRes, error)
 	CheckTxAsync(context.Context, types.RequestCheckTx) (*ReqRes, error)


I believe this gets used in the mempool right? Is it possible (in a follow up PR of course) to modify the usage so we can call CheckTx instead?

Yeah, I tried this, sort of naively, and I think it's possible, but the impact on the mempool is pretty good (mostly because there are a bunch of tests that do assertions/etc. via the callback. I think the best path forward would be to merge this, and then write a shim in the mempool package that does the switch and try and go from there.

cmwaters · 2022-01-19T13:50:01Z

FYI, I followed up with Ethan on slack and he had this response:

this has never been benchmarked but jaes original intention was performance, thinking especially about out-of-process apps. the idea being that txs could quickly sent to the app without having to wait for response
i think its only used for delivertx and checktx. the whole mempool was built around it. does it even work for in-process apps i dont rememver? i think so
so conceivably could be a performance hit in the mempool to use sync abci instead but its also possible the server side abci and sdk arent even making use of it properly and so its all moot lol

williambanfield

This makes me very happy. Not sure I understand the distinction between Flush vs FlushAsync. Didn't look at the mocks.

williambanfield · 2022-01-19T15:28:01Z

abci/client/socket_client.go


-func (cli *socketClient) EchoSync(ctx context.Context, msg string) (*types.ResponseEcho, error) {
+func (cli *socketClient) Echo(ctx context.Context, msg string) (*types.ResponseEcho, error) {
 	reqres, err := cli.queueRequestAndFlushSync(ctx, types.ToRequestEcho(msg))


Should we rename these as well -> queueRequestAndFlush?

In re: Flush vs. FlushAsync. As far as I can tell, there is no difference, but when I tried to get rid of FlushAsync it metastasized. I suspect there is a kernel of simplicity inside that we could use. (Flush appears to be primarily an artifact of the socket client anyway, cf. #6994).

In re: Plumbing methods. Sure, done. But it's harder to be systematic about those.

gofmt -r 'queueRequestAndFlushSync->queueRequestAndFlush' -w abci/client/socket_client.go

creachadair · 2022-01-19T16:53:19Z

FYI, I followed up with Ethan on slack and he had this response:

this has never been benchmarked but jaes original intention was performance, thinking especially about out-of-process apps. the idea being that txs could quickly sent to the app without having to wait for response
i think its only used for delivertx and checktx. the whole mempool was built around it. does it even work for in-process apps i dont rememver? i think so
so conceivably could be a performance hit in the mempool to use sync abci instead but its also possible the server side abci and sdk arent even making use of it properly and so its all moot lol

I agree this is something worth measuring. At the moment I think neither in-process nor out-of-process applications can practically benefit from concurrency here: Even for CheckTx, the socket protocol requires head-of-line blocking because requests cannot be interleaved. And for in-process all client requests share a global mutex. Both of those are issues I think we should address—for performance, certainly, but also to make the control flow easier to understand.

For now, though, I don't think these client-side changes can have any material effect on the performance (in either direction).

This change has two main effects: 1. Remove most of the Async methods from the abci.Client interface. Remaining are FlushAsync, CommitTxAsync, and DeliverTxAsync. 2. Rename the synchronous methods to remove the "Sync" suffix. The rest of the change is updating the implementations, subsets, and mocks of the interface, along with the call sites that point to them.

creachadair requested review from cmwaters, ebuchman, tychoish and williambanfield as code owners January 15, 2022 07:50

creachadair force-pushed the mjf/desync branch 2 times, most recently from a2abfcb to 2fdca21 Compare January 15, 2022 08:04

tac0turtle reviewed Jan 15, 2022

View reviewed changes

creachadair force-pushed the mjf/desync branch 3 times, most recently from f918b4f to 0f65c93 Compare January 16, 2022 21:49

cmwaters approved these changes Jan 18, 2022

View reviewed changes

tychoish mentioned this pull request Jan 18, 2022

mempool: use sync checktx #7615

Closed

creachadair force-pushed the mjf/desync branch 8 times, most recently from 64a95a6 to 3089ceb Compare January 19, 2022 02:37

tychoish approved these changes Jan 19, 2022

View reviewed changes

williambanfield reviewed Jan 19, 2022

View reviewed changes

creachadair force-pushed the mjf/desync branch 2 times, most recently from defedc2 to 825aca7 Compare January 19, 2022 16:24

creachadair force-pushed the mjf/desync branch from 8958e48 to 9163d8c Compare January 19, 2022 16:59

williambanfield approved these changes Jan 19, 2022

View reviewed changes

creachadair force-pushed the mjf/desync branch from 9163d8c to a4018c1 Compare January 19, 2022 17:49

M. J. Fromberger added 2 commits January 19, 2022 10:47

Fix stringly-typed mock stubs.

d8e57a3

Rename helper method.

4b336e6

creachadair force-pushed the mjf/desync branch from a4018c1 to 4b336e6 Compare January 19, 2022 18:47

creachadair merged commit c8e8a62 into master Jan 19, 2022

creachadair deleted the mjf/desync branch January 19, 2022 18:58

creachadair pushed a commit that referenced this pull request Jan 21, 2022

Update pending changelog from #7607.

80486f9

creachadair mentioned this pull request Jan 21, 2022

Update pending changelog from #7607. #7666

Merged

creachadair pushed a commit that referenced this pull request Jan 21, 2022

Update pending changelog from #7607. (#7666)

5a1b5b0

cmwaters mentioned this pull request Sep 14, 2022

abci: port across finalize block from v0.36 #9427

Closed

sergio-mena mentioned this pull request Nov 3, 2022

abci: implement finalize block #9468

Merged

hvanz mentioned this pull request Jun 5, 2023

mempool: Handle concurrent requests in recheck callback cometbft/cometbft#895

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

abci/client: simplify client interface#7607

abci/client: simplify client interface#7607
creachadair merged 3 commits intomasterfrom
mjf/desync

creachadair commented Jan 15, 2022 •

edited

Loading

Uh oh!

tac0turtle left a comment •

edited

Loading

Uh oh!

cmwaters left a comment

Uh oh!

cmwaters Jan 18, 2022

Uh oh!

creachadair Jan 18, 2022

Uh oh!

cmwaters Jan 19, 2022

Uh oh!

cmwaters Jan 18, 2022

Uh oh!

tychoish Jan 18, 2022

Uh oh!

cmwaters commented Jan 19, 2022

Uh oh!

williambanfield left a comment

Uh oh!

williambanfield Jan 19, 2022

Uh oh!

creachadair Jan 19, 2022

Uh oh!

creachadair commented Jan 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -33,35 +33,24 @@ type Client interface {

		// Asynchronous requests
		FlushAsync(context.Context) (*ReqRes, error)

Conversation

creachadair commented Jan 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tac0turtle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmwaters left a comment

Choose a reason for hiding this comment

Uh oh!

cmwaters Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

creachadair Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

cmwaters Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

cmwaters Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

tychoish Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

cmwaters commented Jan 19, 2022

Uh oh!

williambanfield left a comment

Choose a reason for hiding this comment

Uh oh!

williambanfield Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

creachadair Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

creachadair commented Jan 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

creachadair commented Jan 15, 2022 •

edited

Loading

tac0turtle left a comment •

edited

Loading