[Access] Make RegisterID request size configurable #5067

koko1123 · 2023-11-27T21:53:18Z

codecov-commenter · 2023-11-27T21:57:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c344c88) 56.28% compared to head (bc6ebab) 56.22%.
Report is 35 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5067      +/-   ##
==========================================
- Coverage   56.28%   56.22%   -0.07%     
==========================================
  Files         976      977       +1     
  Lines       90985    91176     +191     
==========================================
+ Hits        51212    51260      +48     
- Misses      35966    36105     +139     
- Partials     3807     3811       +4

Flag	Coverage Δ
unittests	`56.22% <100.00%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

peterargue · 2023-11-27T21:57:11Z

cmd/access/node_builder/access_node_builder.go

+			if builder.stateStreamConf.HeartbeatInterval == 0 {
+				return errors.New("state-stream-max-register-values must be greater than or equal to 0")


Suggested change

if builder.stateStreamConf.HeartbeatInterval == 0 {

return errors.New("state-stream-max-register-values must be greater than or equal to 0")

if builder.stateStreamConf.MaxRegisterIdsPerMsg == 0 {

return errors.New("state-stream-max-register-values must be greater than 0")

peterargue · 2023-11-27T21:58:08Z

engine/access/state_stream/backend/backend.go

@@ -38,6 +38,9 @@ type Config struct {
 	// MaxGlobalStreams defines the global max number of streams that can be open at the same time.
 	MaxGlobalStreams uint32

+	// MaxRegisterIdsPerMsg defines the max number of register IDs that can be received in a single request.
+	MaxRegisterIdsPerMsg uint32


nit: go capitalizes acronyms by convention

Suggested change

MaxRegisterIdsPerMsg uint32

MaxRegisterIDsPerMsg uint32

peterargue · 2023-11-27T21:59:23Z

engine/access/state_stream/state_stream.go

@@ -30,6 +30,9 @@ const (

 	// DefaultHeartbeatInterval specifies the block interval at which heartbeat messages should be sent.
 	DefaultHeartbeatInterval = 1
+
+	// DefaultMaxRegisterIdsPerMsg defines the default max number of register IDs that can be received in a single request.
+	DefaultMaxRegisterIdsPerMsg = 100


Suggested change

DefaultMaxRegisterIdsPerMsg = 100

DefaultMaxRegisterIDsPerMsg = 100

peterargue · 2023-11-27T22:01:04Z

engine/access/state_stream/backend/handler_test.go

@@ -541,18 +541,6 @@ func TestGetRegisterValues(t *testing.T) {
 		require.Equal(t, status.Code(err), codes.InvalidArgument)
 	})

-	t.Run("too many register IDs", func(t *testing.T) {


we should keep this test. how about creating it in backend_test

peterargue

few small comments, but otherwise looks good

peterargue · 2023-11-28T04:31:42Z

cmd/access/node_builder/access_node_builder.go

@@ -988,6 +990,9 @@ func (builder *FlowAccessNodeBuilder) extraFlags() {
 			if builder.stateStreamConf.ResponseLimit < 0 {
 				return errors.New("state-stream-response-limit must be greater than or equal to 0")
 			}
+			if builder.stateStreamConf.MaxRegisterIDsPerMsg == 0 {
+				return errors.New("state-stream-max-register-values must be greater than or equal to 0")


this check returns an error if the setting is equal to 0, but the error message says must be greater than or equal to 0. which is correct?

changed to just greater than 0

peterargue · 2023-11-28T04:33:21Z

engine/access/state_stream/backend/backend.go

 	values, err := b.registers.RegisterValues(ids, height)
+	print(err.Error())


Suggested change

print(err.Error())

peterargue · 2023-11-28T04:34:43Z

engine/access/state_stream/backend/backend_executiondata_test.go

@@ -423,3 +437,21 @@ func (s *BackendExecutionDataSuite) TestSubscribeExecutionDataHandlesErrors() {
 		assert.Equal(s.T(), codes.NotFound, status.Code(sub.Err()))
 	})
 }
+
+func (s *BackendExecutionDataSuite) TestGetRegisterValuesErrors() {


is there a happy path test in the backend suite? If not, can you add one?

sideninja · 2023-11-28T18:08:20Z

cmd/access/node_builder/access_node_builder.go

@@ -202,6 +202,7 @@ func DefaultAccessNodeConfig() *AccessNodeConfig {
 			EventFilterConfig:       state_stream.DefaultEventFilterConfig,
 			ResponseLimit:           state_stream.DefaultResponseLimit,
 			HeartbeatInterval:       state_stream.DefaultHeartbeatInterval,
+			MaxRegisterIDsPerMsg:    state_stream.DefaultMaxRegisterIdsPerMsg,


I find the PerMsg a bit confusing, how about?

Suggested change

MaxRegisterIDsPerMsg: state_stream.DefaultMaxRegisterIdsPerMsg,

RegisterIDsRequestLimit: state_stream.DefaultMaxRegisterIdsPerMsg,

sideninja · 2023-11-28T18:09:43Z

cmd/access/node_builder/access_node_builder.go

@@ -931,6 +932,7 @@ func (builder *FlowAccessNodeBuilder) extraFlags() {
 		flags.UintVar(&builder.stateStreamConf.ClientSendBufferSize, "state-stream-send-buffer-size", defaultConfig.stateStreamConf.ClientSendBufferSize, "maximum number of responses to buffer within a stream")
 		flags.Float64Var(&builder.stateStreamConf.ResponseLimit, "state-stream-response-limit", defaultConfig.stateStreamConf.ResponseLimit, "max number of responses per second to send over streaming endpoints. this helps manage resources consumed by each client querying data not in the cache e.g. 3 or 0.5. 0 means no limit")
 		flags.Uint64Var(&builder.stateStreamConf.HeartbeatInterval, "state-stream-heartbeat-interval", defaultConfig.stateStreamConf.HeartbeatInterval, "default interval in blocks at which heartbeat messages should be sent. applied when client did not specify a value.")
+		flags.Uint32Var(&builder.stateStreamConf.MaxRegisterIdsPerMsg, "state-stream-max-register-values", defaultConfig.stateStreamConf.MaxRegisterIdsPerMsg, "maximum number of register ids to include in a single request to the GetRegisters endpoint")


Suggested change

flags.Uint32Var(&builder.stateStreamConf.MaxRegisterIdsPerMsg, "state-stream-max-register-values", defaultConfig.stateStreamConf.MaxRegisterIdsPerMsg, "maximum number of register ids to include in a single request to the GetRegisters endpoint")

flags.Uint32Var(&builder.stateStreamConf.MaxRegisterIdsPerMsg, "state-stream-register-ids-limit", defaultConfig.stateStreamConf.MaxRegisterIdsPerMsg, "limit of register IDs for a single request to the get register endpoint")

sideninja · 2023-11-28T18:11:40Z

engine/access/state_stream/state_stream.go

@@ -30,6 +30,9 @@ const (

 	// DefaultHeartbeatInterval specifies the block interval at which heartbeat messages should be sent.
 	DefaultHeartbeatInterval = 1
+
+	// DefaultMaxRegisterIDsPerMsg defines the default max number of register IDs that can be received in a single request.
+	DefaultMaxRegisterIDsPerMsg = 100


Suggested change

DefaultMaxRegisterIDsPerMsg = 100

DefaultRegisterIDsLimit = 100

…gisters [Access] Make RegisterID request size configurable

[Access] Make RegisterID request size configurable

0c8a69c

koko1123 requested a review from peterargue as a code owner November 27, 2023 21:53

koko1123 requested review from zhangchiqing, sideninja and peterargue and removed request for peterargue November 27, 2023 21:53

peterargue reviewed Nov 27, 2023

View reviewed changes

enhance tests + naming

26fac62

peterargue reviewed Nov 28, 2023

View reviewed changes

changes per comments

e0059d2

koko1123 requested a review from peterargue November 28, 2023 18:04

sideninja reviewed Nov 28, 2023

View reviewed changes

sideninja approved these changes Nov 28, 2023

View reviewed changes

koko1123 added 2 commits November 28, 2023 13:35

cosmetic fixes

c160329

lint

bc6ebab

peterargue approved these changes Nov 28, 2023

View reviewed changes

koko1123 added this pull request to the merge queue Nov 29, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023

peterargue added this pull request to the merge queue Nov 29, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023

peterargue added this pull request to the merge queue Nov 29, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023

koko1123 added this pull request to the merge queue Nov 29, 2023

Merged via the queue into master with commit 9577079 Nov 29, 2023
54 checks passed

koko1123 deleted the amlandeep/refactor-state-sync-registers branch November 29, 2023 23:57

koko1123 added a commit that referenced this pull request Nov 30, 2023

Merge pull request #5067 from onflow/amlandeep/refactor-state-sync-re…

57affd0

…gisters [Access] Make RegisterID request size configurable

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Access] Make RegisterID request size configurable #5067

[Access] Make RegisterID request size configurable #5067

koko1123 commented Nov 27, 2023

codecov-commenter commented Nov 27, 2023 •

edited

Loading

peterargue Nov 27, 2023

peterargue Nov 27, 2023

peterargue Nov 27, 2023

peterargue Nov 27, 2023

peterargue left a comment

peterargue Nov 28, 2023

koko1123 Nov 28, 2023

peterargue Nov 28, 2023

peterargue Nov 28, 2023

sideninja Nov 28, 2023

sideninja Nov 28, 2023

sideninja Nov 28, 2023

		if builder.stateStreamConf.HeartbeatInterval == 0 {
		return errors.New("state-stream-max-register-values must be greater than or equal to 0")

	DefaultMaxRegisterIdsPerMsg = 100
	DefaultMaxRegisterIDsPerMsg = 100

		values, err := b.registers.RegisterValues(ids, height)
		print(err.Error())

	MaxRegisterIDsPerMsg: state_stream.DefaultMaxRegisterIdsPerMsg,
	RegisterIDsRequestLimit: state_stream.DefaultMaxRegisterIdsPerMsg,

	flags.Uint32Var(&builder.stateStreamConf.MaxRegisterIdsPerMsg, "state-stream-max-register-values", defaultConfig.stateStreamConf.MaxRegisterIdsPerMsg, "maximum number of register ids to include in a single request to the GetRegisters endpoint")
	flags.Uint32Var(&builder.stateStreamConf.MaxRegisterIdsPerMsg, "state-stream-register-ids-limit", defaultConfig.stateStreamConf.MaxRegisterIdsPerMsg, "limit of register IDs for a single request to the get register endpoint")

	DefaultMaxRegisterIDsPerMsg = 100
	DefaultRegisterIDsLimit = 100

[Access] Make RegisterID request size configurable #5067

[Access] Make RegisterID request size configurable #5067

Conversation

koko1123 commented Nov 27, 2023

codecov-commenter commented Nov 27, 2023 • edited Loading

Codecov Report

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 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 27, 2023 •

edited

Loading