Add EC replication #1129
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1129
Loading…
Reference in a new issue
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:feat/ec_restore"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Replicate EC chunk to another node
Pull EC chunk from another node
Lost EC chunk restore
Check scenario:
Need to implement EC delete/inhume operations to get correct results.
238aeee47c
to93b2e25f73
c3cfd4d6d6
toc5e5c204c9
c5e5c204c9
to1afdf3409a
WIP: Add EC replicationto Add EC replication@ -77,3 +77,3 @@
Nodes: nodes,
}
s.replicator.HandleTask(ctx, task, &res)
s.replicator.HandleReplicateTask(ctx, task, &res)
HandleReplicationTask
?handle
is a verb,replicate
is a verb tooDone
@ -53,11 +54,6 @@ func (p *Policer) processObject(ctx context.Context, addrWithType objectcore.Add
c := &placementRequirements{}
var numOfContainerNodes int
Seems like a separate fix, unrelated to the commit
Done
@ -0,0 +41,4 @@
c := &placementRequirements{}
checkedNodes := newNodeCache()
for i := range nn {
len(nn)
is currently 1 for all EC policies.Can we simplify the code having this in mind?
Because when it won't be 1, we will also likely have different
EC X.Y
orREP N
descriptors for different indexes in this array.The distinction between
REP
/EC
could be done based on eachnn
element, not on the uppermost level.fixed
@ -0,0 +48,4 @@
default:
}
rd := policy.ReplicaDescriptor(i)
// processNodes replaces rd.GetECDataCount() + rd.GetECParityCount() for len(nn[i]) for valid object type.
Could you elaborate a bit what this comment is trying to explain and why it is here?
Refactored, see last commit.
@ -0,0 +96,4 @@
}
// processECChunk replicates EC chunk if needed.
// Returns True if current chunk should be stored on current node.
It returns a
struct
.Fixed
@ -0,0 +177,4 @@
return false
}
func (p *Policer) collectRequiredECChunks(nodes []netmap.NodeInfo, objInfo objectcore.Info) map[uint32][]netmap.NodeInfo {
By design there is only 1 required chunk to store on the node (EC policy implies
UNIQUE
).Why do we return a map here? It seems the code could be greatly simplified, having this in mind.
Not always: evacuation or not completed replication may lead that chunk may be stored on different chunks.
Evacuation does not affect the set of required chunks in any way.
Agree, current implementation does not affect. But I think that in case of evacuation from a node, EC chunk should move to another node, even if it does not comply with the storage policy.
Yes, but it is moved to a node which should not store it, and I have thought this function determines whether the node should store it?
Another way to look at it: policer must depend only on the network map and placement policy, evacuation is a separate process.
Actually, it may happen in a situation when we have
EC 3.1
and 1 node went offline, so we have 3 nodes in the network map.What is the behaviour in this case?
This function collects all nodes that store required chunks.
Policer will get an error from offline node and will skip object.
1afdf3409a
to76bbd193da
779eb553a9
toed7f18e3f6
851c929b62
to66f540df65
783ed62992
toa78805c555
@ -248,2 +247,4 @@
return remoteReader.Head(ctx, prm)
},
),
policer.WithLocalObjectHeaderFunc(func(ctx context.Context, a oid.Address) (*objectSDK.Object, error) {
Why have you decided to go with 2 func options instead of an interface?
Because this approach has already been used in policer.
@ -97,2 +107,4 @@
return res.Header(), nil
}
func (h *RemoteReader) Get(ctx context.Context, prm *RemoteRequestPrm) (*objectSDK.Object, error) {
I am surprised to see
Get
in thehead
package, given that we also haveget
on the same level.Why is it here?
Moved to objectSvc.
@ -0,0 +293,4 @@
return
}
if objInfo.ECInfo.Total-uint32(len(resolved)) > policy.ReplicaDescriptor(0).GetECParityCount() {
var founded []uint32
Do you mean
found
? It is already a past participle fromfind
,founded
is what happened with Saint-Petersburg in 1703.Fixed
@ -0,0 +323,4 @@
p.log.Error(logs.PolicerFailedToRestoreObject, zap.Stringer("object", parentAddress), zap.Error(err))
return
}
parts, err = c.Split(parentObject, key)
You call
Reconstruct()
, then you callSplit
on the result. These operations are inverse to each other.What is the purpose?
Fixed
a78805c555
to4e772b8eb4
4e772b8eb4
to7727b7fe21
@ -0,0 +168,4 @@
var addr oid.Address
addr.SetContainer(objInfo.Address.Container())
addr.SetObject(indexToObjectID[index])
p.replicator.HandlePullTask(ctx, replicator.Task{
Just for curiosity: do we always need to pull missing chunks from remote node?
processECChunk
setsremoveLocal = true
but a chunk, that is being replicated, is not removed yet untilpullRequiredECChunks
's run. Could a policer try toreconstruct
missing chunk if the local node, by good fortune, keeps required chunks for reconstruction?I suppose this is not a big deal and barely gives a big profit...
By default local node should keep only one chunk.
@ -0,0 +272,4 @@
p.log.Error(logs.PolicerFailedToDecodeECChunkID, zap.Error(err), zap.Stringer("object", parentAddress))
return
}
if existed, exist := chunkIDs[ch.Index]; exist && existed != chunkID {
I found
existed, exist
is not appropriate pair name. You could renamechunkID
toecInfoChunkID
, butexisted
tochunkID
Done
@ -0,0 +273,4 @@
return
}
if existed, exist := chunkIDs[ch.Index]; exist && existed != chunkID {
p.log.Error(logs.PolicerDifferentObjectIDForTheSameECChunk, zap.Stringer("first", existed),
Please, could you give a brief explanation how this could be that chunks related to the same parent may differ by IDs?
A bug that I hope we don't have :)
7727b7fe21
toe9e5ba549e
e9e5ba549e
to436c9f5558