fix(network): batch concurrent tx status lookups in AcceptTxInDBsFilter#1896
fix(network): batch concurrent tx status lookups in AcceptTxInDBsFilter#1896hstarorg wants to merge 3 commits into
Conversation
AcceptTxInDBsFilter.Accept issued up to two serial DB round trips per call, and FSC can have several such calls in flight concurrently while committing a block. Add GetStatuses(ctx, txIDs) to ttxdb/auditdb StoreService, and a statusBatcher that coalesces concurrent Accept lookups within a 300us window into a single batched query per store, instead of one query pair per transaction. Fixes LFDT-Panurus#1872 Signed-off-by: hstarorg <jayhu@sign.global>
require.NoError/ErrorIs must run on the test's main goroutine (go-require) and error assertions should use require, not assert (require-error). Fixes the 4 golangci-lint testifylint findings in filter_test.go and status_batcher_test.go from the PR LFDT-Panurus#1896 CI run. Signed-off-by: hstarorg <jayhu@sign.global>
|
Hi @hstarorg , thanks for submitting this 🙏 |
|
Hi @hstarorg , what's the performance improvement that you have seen with this patch? |
Heads-up: this filter currently isn't reachable from FSC's committerBefore merging — I want to flag something I found while re-reviewing, since it changes what this PR actually delivers at runtime.
transactionFilter, err := n.filterProvider.New(tmsID)
...
committer := n.ch.Committer()
if err := committer.AddTransactionFilter(transactionFilter); err != nil {— were removed from Practical effect: This predates this PR and isn't something it needs to fix by itself, but it seems worth deciding explicitly before merge:
Happy to send the follow-up PR restoring the registration if that's the preferred path. |
Note: the current caller path has that registration removed |
Understood, will do 🙏 |
|
Hi @hstarorg , super interesting. So, the reliance on the FSC's fabric's commit pipeline was removed because that path was too slow. We should not restore therefore it and I would suggest to remove the unused code. Nevertheless, it might be that your patch might still be helpful in |
|
Hi @hstarorg , any follow up on this? Thanks 🙏 |
Thanks for the pointer — that makes sense. I'll rework this PR in that direction: drop the unused Will update the PR shortly. |
AcceptTxInDBsFilter.Accept issued up to two serial DB round trips per call, and FSC can have several such calls in flight concurrently while committing a block. Add GetStatuses(ctx, txIDs) to ttxdb/auditdb StoreService, and a statusBatcher that coalesces concurrent Accept lookups within a 300us window into a single batched query per store, instead of one query pair per transaction.
Fixes #1872