Initial EC implementation #205

Merged
fyrchik merged 4 commits from fyrchik/frostfs-sdk-go:erasure-code into master 2024-03-22 10:14:13 +00:00
Owner

Done:

  • erasurecode package suitable to in node for all tasks

Out of scope:

  • LOCAL statement
  • Split optimizations (parallel pipeline?)
Done: - erasurecode package suitable to in node for all tasks Out of scope: - LOCAL statement - Split optimizations (parallel pipeline?)
fyrchik reviewed 2024-02-28 11:56:31 +00:00
object/type.go Outdated
@ -11,6 +11,7 @@ const (
TypeTombstone
_
TypeLock
TypeECChunk
Author
Owner

Will remove, old iteration.

Will remove, old iteration.
fyrchik marked this conversation as resolved
fyrchik force-pushed erasure-code from 230e6583a4 to 5cbd984458 2024-02-28 11:59:57 +00:00 Compare
fyrchik force-pushed erasure-code from 5cbd984458 to 47156fd14d 2024-02-28 12:03:07 +00:00 Compare
dstepanov-yadro reviewed 2024-03-06 12:38:16 +00:00
netmap/policy.go Outdated
@ -665,0 +682,4 @@
case parser.IEcStmtContext:
res, ok = r.Accept(p).(*netmap.Replica)
default:
continue

This means current statement will be skipped. Why?

This means current statement will be skipped. Why?
Author
Owner

We are interested only in EC and REP statements here and in their order.
Previously it was AllRepStmt, now we get all children and filter all non-REP and non-EC statements, they will be processed below.

We are interested only in EC and REP statements here and in their _order_. Previously it was `AllRepStmt`, now we get all children and filter all non-REP and non-EC statements, they will be processed below.
dstepanov-yadro reviewed 2024-03-06 13:50:27 +00:00
netmap/policy.go Outdated
@ -913,1 +958,4 @@
}
hasEC = hasEC || p.replicas[i].GetECDataCount() != 0 || p.replicas[i].GetECParityCount() != 0
}
if hasEC && !p.unique {

Also we can check that X+Y <= Z for EC X.Y SELECT Z FROM ... policy.

Also we can check that `X+Y <= Z` for `EC X.Y SELECT Z FROM ... ` policy.
Author
Owner

Fixed.

Fixed.
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2024-03-06 13:54:17 +00:00
@ -0,0 +42,4 @@
headerLength := 0
for i := range parts {
if parts[i] == nil {
continue

This looks like invalid usage. I think error must be returned.

This looks like invalid usage. I think error must be returned.
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2024-03-06 13:55:03 +00:00
@ -0,0 +65,4 @@
func (c *Constructor) fillPayload(parts []*objectSDK.Object) {
shards := make([][]byte, len(parts))
for i := range parts {
if parts[i] == nil {

Again, looks like invalid usage.

Again, looks like invalid usage.
Author
Owner

These functions accept collected parts from other nodes, some of the parts may be nil -- these are the ones we need to reconstruct.

These functions accept collected parts from other nodes, some of the parts may be nil -- these are the ones we need to reconstruct.
dstepanov-yadro marked this conversation as resolved
fyrchik reviewed 2024-03-06 17:38:26 +00:00
@ -0,0 +5,4 @@
// "crypto/ecdsa"
// objectSDK "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object"
// "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/transformer"
Author
Owner

Circular dependency, but only for interfaces, will fix.

Circular dependency, but only for interfaces, will fix.
dstepanov-yadro reviewed 2024-03-11 13:50:15 +00:00
@ -58,0 +73,4 @@
var err error
enc, err = erasurecode.NewConstructor(int(p.ECParams.DataCount), int(p.ECParams.ParityCount))
if err != nil {
panic(fmt.Errorf("FIXME: %v", err))

Move to initialize()?

Move to `initialize()`?
Author
Owner

Removed EC from transformer completely.

Removed EC from transformer completely.
dstepanov-yadro marked this conversation as resolved
fyrchik force-pushed erasure-code from 47156fd14d to 3203051e30 2024-03-12 15:03:28 +00:00 Compare
fyrchik force-pushed erasure-code from 3203051e30 to b7a7717fcb 2024-03-20 15:00:36 +00:00 Compare
fyrchik force-pushed erasure-code from b7a7717fcb to cb8fae0c99 2024-03-20 15:04:30 +00:00 Compare
fyrchik force-pushed erasure-code from cb8fae0c99 to fdb07d39ca 2024-03-20 15:07:55 +00:00 Compare
fyrchik force-pushed erasure-code from fdb07d39ca to 3640759e12 2024-03-21 06:45:04 +00:00 Compare
fyrchik force-pushed erasure-code from b04dbf49c7 to 47b8c98b38 2024-03-21 06:58:20 +00:00 Compare
fyrchik requested review from storage-core-committers 2024-03-21 06:58:23 +00:00
fyrchik requested review from storage-core-developers 2024-03-21 06:58:24 +00:00
fyrchik changed title from WIP: initial EC implementation to Initial EC implementation 2024-03-21 06:58:32 +00:00
dstepanov-yadro reviewed 2024-03-21 08:27:09 +00:00
@ -0,0 +31,4 @@
// WriteObject implements the transformer.ObjectWriter interface.
func (t *Target) WriteObject(ctx context.Context, obj *objectSDK.Object) error {
t.c.clear()

Hm, this is already done in Constructor.Split. Also this is Constructor's internal method.

Hm, this is already done in `Constructor.Split`. Also this is `Constructor`'s internal method.
Author
Owner

true, fixed

true, fixed
dstepanov-yadro reviewed 2024-03-21 08:32:58 +00:00
@ -0,0 +12,4 @@
c.clear()
var headerLength int
for i := range parts {

Parts need to be sorted by index according this check in validatePart:

	if ec.Index() != uint32(i) {
		return headerLength, fmt.Errorf("%w: index=%d, ec.index=%d", ErrMalformedSlice, i, ec.Index())
	}

It is important. Need to specify in comment.
But better to create slice copy (it is slice of pointers), sort it and validate against copy.

Parts need to be sorted by index according this check in `validatePart`: ``` if ec.Index() != uint32(i) { return headerLength, fmt.Errorf("%w: index=%d, ec.index=%d", ErrMalformedSlice, i, ec.Index()) } ``` It is important. Need to specify in comment. But better to create slice copy (it is slice of pointers), sort it and validate against copy.
Author
Owner

But this is exactly what Verify is intended to catch: it accepts sorted slice of parts (actually it is more of a testing thing, but can also be used to verify reconstruction).

But this is exactly what `Verify` is intended to catch: it accepts sorted slice of parts (actually it is more of a testing thing, but can also be used to verify reconstruction).
fyrchik force-pushed erasure-code from 47b8c98b38 to 02f8c09b72 2024-03-21 08:37:09 +00:00 Compare
fyrchik force-pushed erasure-code from 02f8c09b72 to 66e1b3e0c3 2024-03-21 08:37:45 +00:00 Compare
fyrchik force-pushed erasure-code from 66e1b3e0c3 to 50e538f27a 2024-03-21 08:55:17 +00:00 Compare
dstepanov-yadro approved these changes 2024-03-21 10:41:24 +00:00
aarifullin reviewed 2024-03-21 13:36:12 +00:00
@ -0,0 +84,4 @@
// If headerLength is not zero it is asserted to be equal in the ec header.
// Otherwise, new headerLength is returned.
func validatePart(parts []*objectSDK.Object, i int, headerLength int) (int, error) {
ec := parts[i].GetECHeader()
Member

What do you think about adding i >= 0 && i < len(parts) check?

What do you think about adding `i >= 0 && i < len(parts)` check?
Author
Owner

This is a private method and is always called in a loop in 2 places, I think we are good here.

This is a private method and is always called in a loop in 2 places, I think we are good here.
aarifullin marked this conversation as resolved
aarifullin reviewed 2024-03-21 14:18:09 +00:00
@ -0,0 +15,4 @@
return nil, err
}
headerShards, err := c.encodeRaw(header)
Member

I've got a question: you separate header and a payload and then marshal the header.
I thought that the number of EC-encoded parts depends on binary size (header_marshalled_size != payload_marshalled_size) but here you expect the header shards number is equal to payload shards number. How come?

I've got a question: you separate header and a payload and then marshal the header. I thought that the number of EC-encoded parts depends on binary size (`header_marshalled_size != payload_marshalled_size`) but here you expect the header shards number is equal to payload shards number. How come?
Author
Owner

Size is not equal, but the number of shards is equal.
It follows, that the size of chunks are not equal.

Size is not equal, but the number of shards is equal. It follows, that the size of chunks are not equal.
aarifullin approved these changes 2024-03-21 14:32:04 +00:00
fyrchik merged commit bd2d350b09 into master 2024-03-22 10:14:13 +00:00
fyrchik deleted branch erasure-code 2024-03-22 10:14:13 +00:00
fyrchik referenced this pull request from a commit 2024-03-22 10:14:15 +00:00
acid-ant approved these changes 2024-03-22 13:49:22 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-sdk-go#205
No description provided.