[Access] Add caching tx results #4598

nozim · 2023-08-03T11:21:53Z

Re: #4585

…s tests

peterargue

thanks for picking this up @nozim! added a few comments.

.gitignore

cmd/access/node_builder/access_node_builder.go

Makefile

engine/access/rpc/backend/backend.go

engine/access/rpc/backend/backend_transactions_test.go

peterargue · 2023-08-10T18:45:26Z

engine/access/rpc/backend/backend_transactions_test.go

+			On("ByBlockID", block.ID()).
+			Return(l, nil)
+
+		suite.historicalAccessClient.


Suggested change

suite.historicalAccessClient.

// assert this is called exactly once for the 2 API calls

suite.historicalAccessClient.

yes, added .Once(

can you also add the comment. it makes it easier for the next person to look at these tests to quickly understand what's happening

nozim · 2023-08-11T09:33:56Z

@peterargue thank you for review, fixes on the way ;)

kc1116 · 2023-08-24T16:52:41Z

@nozim please ping me when all feedback has been addressed

nozim · 2023-08-28T09:56:49Z

@kc1116 could you please rerun ci, fixed last linter error.

nozim · 2023-08-29T09:37:48Z

@kc1116 @peterargue kindly run the ci. Fixed all the errors.

nozim · 2023-08-31T11:29:09Z

flow-go/network/test test are failing on master as well. Kindly let me know when they are fixed and I'll sync with master.
@kc1116 @peterargue

peterargue

Thanks @nozim this looks great. Also thanks for cleaning up the backend inputs. the list was getting pretty long.

Added a few small comments, but I think this is ready to go.

peterargue · 2023-09-01T04:34:03Z

engine/access/rpc/backend/backend.go

+	if params.TxResultCacheSize > 0 {
+		txResCache, err = lru2.New[flow.Identifier, *access.TransactionResult](int(params.TxResultCacheSize))
+		if err != nil {
+			params.Log.Fatal().Err(err).Msg("failed to init cache for transaction results")
+		}
+	}


my 2cents is to use the value 0 to indicate that the cache is disabled.

I originally requested that it be disabled by default to start and we can enable it in the future. It's realistically only useful on nodes where users frequently lookup non-existent TXs, so only infrastructure providers will realistically get value from it.

engine/access/rpc/backend/backend_block_details.go

peterargue · 2023-09-01T04:36:39Z

engine/access/rpc/backend/backend_transactions.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"time"

+	lru2 "github.com/hashicorp/golang-lru/v2"


Let's move this to a separate issue. We're already using this library within the access api, and adding a new herocache type is non-trivial.

peterargue · 2023-09-01T04:39:04Z

engine/access/rpc/backend/backend_transactions.go

+		}
+
+		// Tx not found. If we have historical Sporks setup, lets look through those as well
+		if b.txResultCache != nil {


@kc1116 this cache is here to avoid unnecessary lookups on the historic nodes since they are very expensive. the linked issue has some more context

peterargue · 2023-09-01T04:45:28Z

engine/access/rpc/backend/node_communicator.go

+	call func(id *flow.Identity) error,
+	shouldTerminateOnError func(node *flow.Identity, err error) bool,


please use the original types. They help document the callbacks

this was added to avoid import cycle due to mock -> backend -> mock dependency.

If i add them back I'll have to amend makefile mockgen which will differ from conventional way 😊

ok. then please move the comments from the function types into the CallAvailableNode godocs so devs can easily see the usage details. You can remove those function types if they're unused.

peterargue · 2023-09-01T04:48:42Z

engine/access/rpc/backend/backend_transactions_test.go

+			Params{
+				State:                suite.state,
+				CollectionRPC:        suite.colClient,
+				Blocks:               suite.blocks,
+				Transactions:         suite.transactions,
+				ExecutionReceipts:    suite.receipts,
+				ChainID:              suite.chainID,
+				AccessMetrics:        metrics.NewNoopCollector(),
+				MaxHeightRange:       DefaultMaxHeightRange,
+				Log:                  suite.log,
+				SnapshotHistoryLimit: DefaultSnapshotHistoryLimit,
+				Communicator:         suite.communicator,
+			})


nit: you could add a defaultParams() method which returns this default set. That way you don't need to repeat this block of code in each test. If you need to use a specific value for some fields, you can just update the param set before creating the backend

peterargue · 2023-09-01T04:51:45Z

engine/access/rpc/backend/backend_transactions_test.go

+			On("ByBlockID", block.ID()).
+			Return(l, nil)
+
+		suite.historicalAccessClient.


can you also add the comment. it makes it easier for the next person to look at these tests to quickly understand what's happening

peterargue · 2023-09-01T04:52:20Z

engine/access/rpc/backend/backend_transactions_test.go

+		suite.Require().Equal(flow.TransactionStatusExecuted, resp.Status)
+		suite.Require().Equal(uint(flow.TransactionStatusExecuted), resp.StatusCode)
+
+		resp2, err := backend.GetTransactionResult(context.Background(), tx.ID(), block.ID(), coll.ID())


also here

Suggested change

resp2, err := backend.GetTransactionResult(context.Background(), tx.ID(), block.ID(), coll.ID())

// the second call should return the correct response without generating a call to the historic node

resp2, err := backend.GetTransactionResult(context.Background(), tx.ID(), block.ID(), coll.ID())

peterargue · 2023-09-01T04:52:47Z

engine/access/rpc/backend/backend_transactions_test.go

+func (suite *Suite) TestGetTransactionResultCacheNonExistent() {
+	suite.withGetTransactionCachingTestSetup(func(block *flow.Block, tx *flow.Transaction) {
+
+		suite.historicalAccessClient.


peterargue · 2023-09-01T05:05:16Z

flow-go/network/test test are failing on master as well. Kindly let me know when they are fixed and I'll sync with master. @kc1116 @peterargue

There's a PR in the works to make that suite more reliable. Once that's merged and you pull the changes, it should pass.

nozim · 2023-09-01T11:51:12Z

@peterargue I'd be more than happy to address all your last inputs in following PR if you don't mind.

nozim · 2023-09-05T16:15:28Z

@kc1116 kindly rerun the ci. should be fixed now.

peterargue · 2023-09-05T16:17:16Z

@peterargue I'd be more than happy to address all your last inputs in following PR if you don't mind.

feel free to skip the nit, but please do address my notes about adding comments to the tests and node_communicator.go

nozim · 2023-09-05T17:18:56Z

@peterargue kindly check 69b4339 🙏

nozim added 5 commits August 3, 2023 12:00

4585 Add more testable backend constructor with unit tests

0f82f8c

4585: Add mock file generation for NodeCommunicator

3d23bcf

4585: Add simple lru caching for transaction results and refactor it'…

f018aca

…s tests

4585: Ignore mock file

8dd08db

4585: Fix goimport issues

b3865c4

nozim requested a review from peterargue as a code owner August 3, 2023 11:21

nozim changed the title ~~4585 add caching tx results~~ [Access] Add caching tx results Aug 3, 2023

nozim added 5 commits August 3, 2023 12:24

Merge branch 'master' into 4585-add-caching-tx-results

ffd8cf7

Merge branch 'master' into 4585-add-caching-tx-results

ecdafd7

Refactor backend constructor, replace old

d3a5145

Merge branch 'master' into 4585-add-caching-tx-results

cba02b4

Fix goimports

985b558

peterargue reviewed Aug 10, 2023

View reviewed changes

nozim added 16 commits August 12, 2023 11:55

Merge branch 'master' into 4585-add-caching-tx-results

eb0257c

Fix failing tests

42897b2

Update backend constructor annotation

9935534

Update mock call

1e60d8d

Remove unused mock setup in backend transactions

0d51ec3

Refine backend transaction tests

da2c6be

Simplify backend transactiont tests

de50262

Rename test case

01d423e

Require mock to be called exactly once

43193d6

Refactor Access backend constructor

77e65b4

Fix backend tests

ed1b3b2

Revert comment changes, rename colliding status variable name

4cfeafc

Revert comment changes, rename colliding status variable

fef906c

Rename Backend constructor params structure

5b31bfa

Introduce TxResultCacheParam for configuration

ae8ccaa

Initialise TxResultCacheSize from access node flags

37bb9a3

fix goimports

fd03e0f

nozim force-pushed the 4585-add-caching-tx-results branch from e2eaf25 to fd03e0f Compare August 24, 2023 21:50

nozim added 3 commits August 24, 2023 22:51

Merge branch 'master' into 4585-add-caching-tx-results

1769e48

update state stream mock imports

b92cb81

Fix observer builder formatting

7acb9ed

nozim mentioned this pull request Aug 28, 2023

Add Transaction Results Caching + Unit testing + Refactoring onflow/developer-grants#195

Open

Merge branch 'master' into 4585-add-caching-tx-results

7ac37b1

Merge branch 'master' into 4585-add-caching-tx-results

5475b83

peterargue approved these changes Sep 1, 2023

View reviewed changes

Merge branch 'master' into 4585-add-caching-tx-results

3a55308

nozim requested a review from peterargue September 1, 2023 11:53

franklywatson and others added 2 commits September 1, 2023 09:25

Merge branch 'master' into 4585-add-caching-tx-results

05d817b

Merge branch 'master' into 4585-add-caching-tx-results

1a40b99

Remove unused CallAvailableNode types and add godoc for it's parameters

69b4339

nozim and others added 3 commits September 5, 2023 18:50

Merge branch 'master' into 4585-add-caching-tx-results

7347a32

Merge branch 'master' into 4585-add-caching-tx-results

4da0222

Fix linter errors

d2b0826

kc1116 approved these changes Sep 5, 2023

View reviewed changes

franklywatson added this pull request to the merge queue Sep 5, 2023

Merged via the queue into onflow:master with commit 3cc4c32 Sep 5, 2023
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Access] Add caching tx results #4598

[Access] Add caching tx results #4598

nozim commented Aug 3, 2023 •

edited

Loading

peterargue left a comment

peterargue Aug 10, 2023

nozim Aug 21, 2023

nozim Aug 21, 2023

peterargue Sep 1, 2023

nozim commented Aug 11, 2023

kc1116 commented Aug 24, 2023

nozim commented Aug 28, 2023

nozim commented Aug 29, 2023

nozim commented Aug 31, 2023

peterargue left a comment

peterargue Sep 1, 2023

peterargue Sep 1, 2023

peterargue Sep 1, 2023

peterargue Sep 1, 2023

nozim Sep 2, 2023

nozim Sep 2, 2023 •

edited

Loading

peterargue Sep 5, 2023

peterargue Sep 1, 2023

peterargue Sep 1, 2023

peterargue Sep 1, 2023

peterargue Sep 1, 2023

peterargue commented Sep 1, 2023

nozim commented Sep 1, 2023

nozim commented Sep 5, 2023

peterargue commented Sep 5, 2023

nozim commented Sep 5, 2023 •

edited

Loading

	suite.historicalAccessClient.
	// assert this is called exactly once for the 2 API calls
	suite.historicalAccessClient.

		call func(id *flow.Identity) error,
		shouldTerminateOnError func(node *flow.Identity, err error) bool,

	resp2, err := backend.GetTransactionResult(context.Background(), tx.ID(), block.ID(), coll.ID())
	// the second call should return the correct response without generating a call to the historic node
	resp2, err := backend.GetTransactionResult(context.Background(), tx.ID(), block.ID(), coll.ID())

[Access] Add caching tx results #4598

[Access] Add caching tx results #4598

Conversation

nozim commented Aug 3, 2023 • edited Loading

peterargue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nozim commented Aug 11, 2023

kc1116 commented Aug 24, 2023

nozim commented Aug 28, 2023

nozim commented Aug 29, 2023

nozim commented Aug 31, 2023

peterargue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nozim Sep 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterargue commented Sep 1, 2023

nozim commented Sep 1, 2023

nozim commented Sep 5, 2023

peterargue commented Sep 5, 2023

nozim commented Sep 5, 2023 • edited Loading

nozim commented Aug 3, 2023 •

edited

Loading

nozim Sep 2, 2023 •

edited

Loading

nozim commented Sep 5, 2023 •

edited

Loading