Implement the new indexerLedgerForEval interface in go-algorand
#652
Conversation
tolikzinovyev
force-pushed
the
tolik/batching-refactor
branch
from
b2daa87 to
73b55f2
Compare
Codecov Report
@@ Coverage Diff @@
## develop #652 +/- ##
===========================================
+ Coverage 51.85% 51.98% +0.12%
===========================================
Files 23 23
Lines 3560 3557 -3
===========================================
+ Hits 1846 1849 +3
+ Misses 1462 1457 -5
+ Partials 252 251 -1
Continue to review full report at Codecov.
|
tolikzinovyev
force-pushed
the
tolik/batching-refactor
branch
from
6b42753 to
a75ebfc
Compare
|
I think we can now update go-algorand. |
are you referring to the changes in algorand/go-algorand#2846 ? |
|
No, we were pointing go-algorand to its release branch. Now that we have released a beta Indexer, we can point go-algorand to master again. |
tolikzinovyev
force-pushed
the
tolik/batching-refactor
branch
2 times, most recently
from
b8357e6 to
8249f39
Compare
tolikzinovyev
force-pushed
the
tolik/batching-refactor
branch
from
8249f39 to
4e3a284
Compare
winder
left a comment
winder
left a comment
There was a problem hiding this comment.
Looks great! Just had a couple questions.
| } | ||
| return accountData, round, nil | ||
| // GetAssetCreator is part of go-algorand's indexerLedgerForEval interface. | ||
| func (l LedgerForEvaluator) GetAssetCreator(indices map[basics.AssetIndex]struct{}) (map[basics.AssetIndex]ledger.FoundAddress, error) { |
There was a problem hiding this comment.
Was there any thought about organizing this so that it could be batched along with the account lookup?
There was a problem hiding this comment.
It could be done, but a couple of additional RTTs do not make a difference for amazon aurora if we want to optimize TPS. The RTT is < 1ms, so for large blocks it is negligible. However, if we also want to optimize the catchup time, we should revisit this.
There was a problem hiding this comment.
What about the query duration? Sounds like the RTT isn't really a factor, but running each of these in parallel could have some value.
There was a problem hiding this comment.
I don't know if we can influence parallelism and gain performance. One thing that comes to mind is read requests going to different read replicas of aurora can help, but we don't have control over it, and I'm not sure the load balancer is smart enough. In addition, other databases (regular postgres) might have worse performance because of parallelism (one thing that comes to mind is HDD where parallel reading causes too many head jumps so the overall performance is worse, not sure how it works for SSD). Also, if we want to parallelize, we should start with parallelizing account query from the account table, as that is longest part of reading data.
| return fmt.Errorf( | ||
| "AddBlock() cannot find proto version %s", block.BlockHeader.CurrentProtocol) | ||
| } | ||
| proto.EnableAssetCloseAmount = true |
There was a problem hiding this comment.
Do you think things like this could move into algod also?
There was a problem hiding this comment.
I don't see a good reason. What about you?
There was a problem hiding this comment.
The main reason would be to keep consensus param "magic" as close to the source as possible. If we have an interface in algod designed for Indexer, shouldn't we use it?
There was a problem hiding this comment.
I don't have a strong opinion. @tsachiherman maybe does.
|
Thanks for review! |
Summary
This significantly simplifies the code in Indexer, and paves a way to querying asset/app creators in a batch.
Test Plan
New tests for batch querying of asset/app creators.