-
Notifications
You must be signed in to change notification settings - Fork 79
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
services: add new block fetching from NeoFS service #3515
Conversation
AliceInHunterland
commented
Jul 16, 2024
•
edited by AnnaShaleva
Loading
edited by AnnaShaleva
- documentation update
- tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3515 +/- ##
==========================================
- Coverage 85.79% 85.21% -0.59%
==========================================
Files 330 333 +3
Lines 38536 38953 +417
==========================================
+ Hits 33062 33192 +130
- Misses 3928 4192 +264
- Partials 1546 1569 +23 ☔ View full report in Codecov by Sentry. |
case ps[0] == headerCmd: | ||
res.ReadCloser, err = getHeader(ctx, s, c, objectAddr) | ||
case ps[0] == hashCmd: | ||
res.ReadCloser, err = getHash(ctx, s, c, objectAddr, ps[1:]...) |
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.
Not needed.
d4e0673
to
db88338
Compare
db88338
to
34f4cf4
Compare
34f4cf4
to
616393d
Compare
29559e3
to
416c04e
Compare
@@ -105,6 +106,13 @@ func NewCommands() []*cli.Command { | |||
Action: dumpDB, | |||
Flags: cfgCountOutFlags, | |||
}, | |||
{ | |||
Name: "dump-bin", |
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.
Maybe not dump
, but fetch
.
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.
@roman-khimov, why fetch
? This command saves dump of every block to a separate file, it doesn't fetch them from the network.
pkg/network/server.go
Outdated
if s.ServerConfig.NeoFSCfg.Restore { | ||
done := make(chan struct{}) | ||
s.blockFetcher.Start( | ||
func() { | ||
s.log.Info("BlockFetcher service finished") | ||
close(done) | ||
}) | ||
<-done |
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.
may we postpone the StateSync integration to a separate issue and prohibit to enable BlockFetcher and StateSync simultaneously in the node config?
Let's do it here, header objects were always meant to be stored as well.
We can decouple some of the network server logic to detect sync/non-sync and then try various fetching strategies (NeoFS/P2P) depending on the state. But NeoFS sync is mostly for the initial one, then P2P is sufficient. Anyway, bqueue.Queue
should be reused, it handles blocks nicely already.
if err != nil { | ||
return nil, err | ||
} | ||
rc, err := neofs.GetWithClient(ctx, bfs.client, privateKey, u, bfs.Nodes[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.
pool
?
2c6f8ca
to
cabf922
Compare
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 first part of comments, I'll left the second part soon.
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.
We're missing a retry mechanism (for cases when some object was failed to load or when some block was failed to be added into the blockqueue), but it will be implemented later. Let's now focus on finalisation of two BlockFetcher operation modes to be able to benchmark them.
b500622
to
1335764
Compare
Still have something to fix in BlockFetcher before benchmarks. |
70cfc5a
to
cdbaff1
Compare
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.
@AliceInHunterland BlockFetcher is missing at least some basic tests for those things that can be tested without network. E.g. isActive
, service constructors, default parameters validation and etc.
If Blocking mode is on PutBlock will block until there is enough space in the queue. Co-authored-by: Anna Shaleva <[email protected]> Signed-off-by: Ekaterina Pavlova <[email protected]>
e035502
to
ab1a11b
Compare
ab1a11b
to
bfb9120
Compare
|
d41946b
to
9f186ce
Compare
IDs in binary form ordered by block index. Each index file is marked with the | ||
following attributes: | ||
- index file identifier with consecutive file index value (`oid:0`) | ||
- the number of OIDs included into index file (`size:128000`) |
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.
@roman-khimov, do we need size:128000
attribute?
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.
Just as an option, various index files size for different schemas.
@roman-khimov, ready for review&merge from my side. |
return fmt.Errorf("failed to fetch '%s' object with index %d: %w", bfs.cfg.IndexFileAttribute, startIndex, err) | ||
} | ||
|
||
err = bfs.streamBlockOIDs(oidsRC, int(skip)) |
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.
One possible problem that I see with current design is that when we're getting OIDs from index files, a connection to storage node remains opened. We read OID files step-by-step and if destination channel is full then streamBlockOIDs
will wait with opened connection to NeoFS storage node. Timeout may happen here.
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.
Retry probably will help?
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.
It's a design problem.
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.
Ref. #3585.
5027493
to
1711968
Compare
@AliceInHunterland, windows tests are failing.
|
Dump blocks (starting with the genesis or specified block) to the directory in binary format. Signed-off-by: Ekaterina Pavlova <[email protected]>
Could be used for operation with neofs. Signed-off-by: Ekaterina Pavlova <[email protected]>
1711968
to
a711315
Compare
@AliceInHunterland, another test is failing on Win:
|
a711315
to
d884ae9
Compare
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.
Otherwise LGTM.
Close #3496 Co-authored-by: Anna Shaleva <[email protected]> Signed-off-by: Ekaterina Pavlova <[email protected]>
d884ae9
to
0b31a29
Compare