-
Notifications
You must be signed in to change notification settings - Fork 180
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
[Access] Use local event for AccessAPI get events endpoints #4851
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4851 +/- ##
==========================================
- Coverage 56.42% 54.46% -1.96%
==========================================
Files 981 815 -166
Lines 93053 75174 -17879
==========================================
- Hits 52504 40943 -11561
+ Misses 36662 31265 -5397
+ Partials 3887 2966 -921
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7a38f06
to
9621040
Compare
eventType string, | ||
requiredEventEncodingVersion entities.EventEncodingVersion, | ||
) ([]flow.BlockEvents, []*flow.Header, error) { | ||
target := flow.EventType(eventType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to validate the event type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the target
is only used for effectively a string comparison, so it's pretty benign, but I can add it.
|
||
missing := make([]*flow.Header, 0) | ||
resp := make([]flow.BlockEvents, 0) | ||
for _, header := range blockHeaders { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to restrict the number of blocks to query events from? Or the caller has checked that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the caller checks that
here
flow-go/engine/access/rpc/backend/backend_events.go
Lines 52 to 55 in ce9c12a
if rangeSize > uint64(b.maxHeightRange) { | |
return nil, status.Errorf(codes.InvalidArgument, | |
"requested block range (%d) exceeded maximum (%d)", rangeSize, b.maxHeightRange) | |
} |
and here
flow-go/engine/access/rpc/backend/backend_events.go
Lines 109 to 111 in ce9c12a
if uint(len(blockIDs)) > b.maxHeightRange { | |
return nil, status.Errorf(codes.InvalidArgument, "requested block range (%d) exceeded maximum (%d)", len(blockIDs), b.maxHeightRange) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for me
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
[Access] Use local event for AccessAPI get events endpoints
Closes: #4751
Use locally indexed events for
GetEvents*
endpoints