-
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] Add implementation for events data providers and account statuses data providers #6766
[Access] Add implementation for events data providers and account statuses data providers #6766
Conversation
…:The-K-R-O-K/flow-go into AndriiDiachuk/6588-events-data-provider
…:The-K-R-O-K/flow-go into AndriiDiachuk/6588-events-data-provider
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6766 +/- ##
==========================================
+ Coverage 41.01% 41.09% +0.08%
==========================================
Files 2104 2107 +3
Lines 185030 185286 +256
==========================================
+ Hits 75892 76151 +259
+ Misses 102767 102733 -34
- Partials 6371 6402 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
engine/access/rest/websockets/data_providers/events_provider.go
Outdated
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/events_provider.go
Outdated
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/events_provider.go
Outdated
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/events_provider_test.go
Outdated
Show resolved
Hide resolved
…:The-K-R-O-K/flow-go into AndriiDiachuk/6588-events-data-provider
…:The-K-R-O-K/flow-go into AndriiDiachuk/6588-events-data-provider
…The-K-R-O-K/flow-go into AndriiDiachuk/6587-accounts-data-provider
…achuk/6588-events-data-provider
engine/access/rest/websockets/data_providers/account_statuses_provider.go
Outdated
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/account_statuses_provider.go
Outdated
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/events_provider.go
Outdated
Show resolved
Hide resolved
…The-K-R-O-K/flow-go into AndriiDiachuk/6588-events-data-provider
engine/access/rest/websockets/data_providers/account_statuses_provider.go
Outdated
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/account_statuses_provider_test.go
Outdated
Show resolved
Hide resolved
engine/access/rest/websockets/data_providers/events_provider.go
Outdated
Show resolved
Hide resolved
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 to me
blocksSinceLastMessage = 0 | ||
} |
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.
I think this should be outside of the check.
blocksSinceLastMessage = 0 | |
} | |
} | |
blocksSinceLastMessage = 0 |
otherwise, it could behave strangely. for example, if heartbeatInterval
is 5 and there are 4 empty blocks in a row, blocksSinceLastMessage
would be 4. Then if the next block contained events, a message would be sent without resetting blocksSinceLastMessage
, so on the next empty block, a heartbeat message would be sent.
looks like we have this bug in all of the places. I opened #6837 to fix this everywhere. you can skip it in this PR
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.
LGTM!
Closes: #6588, #6587
In this PR
EventsDataProvider
andAccountStatusesDataProvider
were implemented.