node/meta: Use neo-go's new notification API #3164
Conversation
carpawell
requested review from
roman-khimov and
cthulhu-rider
as code owners
carpawell
force-pushed
the
fix/meta/get-notifications-from-block
branch
from
6792706 to
67929c2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3164 +/- ##
==========================================
+ Coverage 23.01% 23.03% +0.01%
==========================================
Files 756 757 +1
Lines 60210 60210
==========================================
+ Hits 13860 13868 +8
+ Misses 45357 45349 -8
Partials 993 993 ☔ View full report in Codecov by Sentry. |
pkg/services/meta/block.go
Outdated
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| func (m *Meta) blockFetcher(ctx context.Context, buff <-chan *block.Header) { |
There was a problem hiding this comment.
could u pls move these methods to a separate source file in a separate patch? this would easify the review
There was a problem hiding this comment.
btw, i cost me additional 30+ mins for rebasing, conflicting, fixing, and rechecking additional threads below. this PR changes logic in general so old functions become separate things (it is by blocks now), but still PR is 200- lines, imo it was not worth it but it is already done
There was a problem hiding this comment.
ive no idea how u could face any conflicts with this change, but thx anyway
There was a problem hiding this comment.
because i was moving parts that should be changed to fix other threads. change it once in the original code, and change it second time in the moving commit. moving commit cannot be first because there was no "block" logic before and imo it was a good change as a single commit that changed everything
| @@ -32,6 +33,17 @@ type ContainerLister interface { | |||
| List() (map[cid.ID]struct{}, error) | |||
| } | |||
|
|
|||
| // wsClient is for test purposes only. | |||
| type wsClient interface { | |||
| GetBlockNotifications(blockHash util.Uint256, filters ...*neorpc.NotificationFilter) (*result.BlockNotifications, error) | |||
There was a problem hiding this comment.
suggest to write docs incl. what behavior Meta expects for blocks w/o notifications - error of both nil?
There was a problem hiding this comment.
what docs? this is literally simplification for tests. if there is either a test http server that can be mocked or we do not use tests at all, this interface does not exist. this is literally rpcclient.WSClient. docs will not save us if neo-go decides to change behavior. i do not plan to have any self written implementation here ever
There was a problem hiding this comment.
ik why it's needed, idk how GetBlockNotifications interface behaves in mentioned case and wanna highlight this
neo-go decides to change behavior
it definitely wont change the behavior intentionally
There was a problem hiding this comment.
will some reference to rpcclient.WSClient be enough in the comment? cause i do not understand you. nothing should implement it and it declares nothing for no one, it is literally rpcclient.WSClient but it is hard to mock it for tests. and i prefer this interface not to exist, but it would decrease coverage. i believe we will come to httptest package usage but not now
There was a problem hiding this comment.
yeah, u may state that this is a part of WSClient interface consumed by Meta
but 1st of all i wanna realize what is returned - 404 error, both nil or non-nil with empty fields?
It has already been implemented via deleted container notifications. Signed-off-by: Pavel Karpy <[email protected]>
carpawell
force-pushed
the
fix/meta/get-notifications-from-block
branch
from
67929c2 to
80389fa
Compare
Receive object notifications by block and process them in sync, not independently. This is important for a consistent MPT state. Strictly speaking, it was a bug before but it was a bug that was impossible to fix without neo-go's new `getblocknotifications` API available with 0.108.0 release. Closes #3142. Signed-off-by: Pavel Karpy <[email protected]>
Also rename `container.go` to `containers.go` since we already have `blocks.go` and `notifications.go` with the same meaning. Signed-off-by: Pavel Karpy <[email protected]>
carpawell
force-pushed
the
fix/meta/get-notifications-from-block
branch
from
80389fa to
f85af2f
Compare
|
Also fixed races. |
Receive object notifications by block and process them in sync, not independently. This is important for a consistent MPT state. Strictly speaking, it was a bug before but it was a bug that was impossible to fix without neo-go's new
getblocknotificationsAPI available with 0.108.0 release. Closes #3142.