Skip to content

indexer: move deduplication functionality purely to the kvindexer#9473

Merged
cmwaters merged 8 commits intomainfrom
cal/deduplicate_txs
Oct 7, 2022
Merged

indexer: move deduplication functionality purely to the kvindexer#9473
cmwaters merged 8 commits intomainfrom
cal/deduplicate_txs

Conversation

@cmwaters
Copy link
Contributor

Closes: #9460

The deduplication logic which prevents older failed transactions from overwriting newer successful transactions called the Get method on the TxIndexer. For postgresql, this wasn't implemented and would always return an error. Errors were only logged which meant that the postgresql would batch nil for that height` and then continue. This resolves the issue by moving the logic purely within the kvindexer

@cmwaters cmwaters requested a review from ebuchman as a code owner September 21, 2022 13:50
@cmwaters cmwaters requested a review from a team September 21, 2022 13:50
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Could you please add a test here to test specifically for the problem outlined in #9460?

@cmwaters
Copy link
Contributor Author

Could you please add a test here to test specifically for the problem outlined in #9460?

I've added a test. As part of it, I've added a flag in the constructor which will terminate the indexing service upon the first error. That way I can detect that the postgres indexer has successfully processes the transactions

Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com>
@cmwaters cmwaters added S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x S:backport-to-v0.37.x Tell mergify to backport the PR to v0.37.x labels Sep 30, 2022
batch.Ops, err = DeduplicateBatch(batch.Ops, is.txIdxr)
if err != nil {
is.Logger.Error("deduplicate batch", "height", height)
is.Logger.Info("indexed block exents", "height", height)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the error no longer needed here. Is this only because we are getting rid of the "deduplicate batch"

@cmwaters
Copy link
Contributor Author

cmwaters commented Oct 4, 2022

I will add the changelog for the v0.34 and v0.37 backports

err = eventBus.PublishEventTx(types.EventDataTx{TxResult: *txResult2})
require.NoError(t, err)

time.Sleep(100 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

We really need to refactor our abstractions in such a way that allows us to avoid time.Sleep in future when "awaiting" some kind of asynchronous consequence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, currently the indexer is such an opaque system to debug. One improvement I thought of would be to track the latest height indexed, then we could call to make sure that it had indexed these heights without error

Height: 1,
Index: uint32(1),
Tx: types.Tx("bar"),
Result: abci.ResponseDeliverTx{Code: 1},
Copy link
Contributor

Choose a reason for hiding this comment

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

The Postgres indexer would error in this case, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, previously it would error

@pratikbin
Copy link

pratikbin commented Jan 2, 2023

I couldn't find the docs for creating initial migration or relation in DB

adrianbrink pushed a commit to heliaxdev/tendermint that referenced this pull request May 23, 2023
…ndermint#9473) (#228)

(cherry picked from commit 4fd19a2)

Co-authored-by: Callum Waters <cmwaters19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x S:backport-to-v0.37.x Tell mergify to backport the PR to v0.37.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When using postgres indexer, TxResults for blocks where any transaction has an error code are not stored

5 participants