Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further optimize blockchain transaction inserts to DB #1354

Merged
merged 12 commits into from
Jul 5, 2023

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Jun 24, 2023

In PR chain with #1343

  • Creates a new txwriter package that depends on both operations and txcommon to do batched inserts
    • Dispatches to a pool of background workers
    • Assigns a new ID to each transaction in the batch
    • Splits to transactions with/without idempotencyKey set
    • Attempts to insert all with new InsertTransactions multi-insert function in DB layer
    • Handles partial results on the idempotencyKey set TX, by querying those idempotency keys
    • Compares ID of the transactions in the DB, with the inserts, to find the duplicates
    • Inserts the operations with a new multi-insert function in the DB layer (these always have new IDs)
  • Updates contracts package to use ^^^ instead of one-by-one insertion of tx+ops
    • The inbound thread for invoke/deploy requests doesn't even have a DB transaction now (RunAsGroup call removed)
    • Creation of the core.Operation moved up the function, as need to be passed in
    • Idempotency handling to resubmit operations in the case of a clash, unchanged
  • Updates blockchain connector interface to split parsing with execution
    • We were compiling the FFI schema twice per transaction in the blockchain connector
  • Adds caching to contracts package
    • For requests referencing an FFI: FFI ID + methodPath -> FFIMethod + []*FFIError
    • For all requests: Hash of compiled JSON Schema, to the JSON Schema object
    • Also builds a hash-of-hashes across the combination of method+errors, which is used to cache the result from the blockchain connector validation

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
…ma validations

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2023

Codecov Report

Merging #1354 (e40ec89) into main (f286d82) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##             main    #1354    +/-   ##
========================================
  Coverage   99.98%   99.98%            
========================================
  Files         314      315     +1     
  Lines       21581    21902   +321     
========================================
+ Hits        21577    21898   +321     
  Misses          2        2            
  Partials        2        2            
Impacted Files Coverage Δ
internal/data/message_writer.go 100.00% <ø> (ø)
internal/assets/token_approval.go 100.00% <100.00%> (ø)
internal/assets/token_pool.go 100.00% <100.00%> (ø)
internal/assets/token_transfer.go 100.00% <100.00%> (ø)
internal/blockchain/ethereum/ethereum.go 100.00% <100.00%> (ø)
internal/blockchain/fabric/fabric.go 100.00% <100.00%> (ø)
internal/broadcast/manager.go 100.00% <100.00%> (ø)
internal/contracts/manager.go 100.00% <100.00%> (ø)
internal/contracts/operations.go 100.00% <100.00%> (ø)
internal/coreconfig/coreconfig.go 100.00% <100.00%> (ø)
... and 7 more

@peterbroadhurst peterbroadhurst marked this pull request as ready for review June 25, 2023 21:42
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@nguyer
Copy link
Contributor

nguyer commented Jul 5, 2023

@peterbroadhurst It does look like we have a coverage miss here: https://app.codecov.io/gh/hyperledger/firefly/pull/1354/blob/internal/operations/context.go

Comment on lines 422 to 426
if _, ok := err.(*sqlcommon.IdempotencyError); ok {
if op != nil {
// Idempotency key clash but we resubmitted an initialized operation? Return 20x, not 409
return op, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know for sure that this is a resubmitted operation at this point? Or is it possible that we would still hit this in return in a case where the user re-used an idempotency key and we should have rejected it earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question... digging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - I've worked through the code, and did find (existing I think in all cases) issues in how the TXID was being propagated, and a weirdness on an interface that returned a single operation even though there might be more.

Commit added e40ec89

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@nguyer nguyer merged commit e351e81 into hyperledger:main Jul 5, 2023
14 checks passed
@nguyer nguyer deleted the invoke-insert branch July 5, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants