adds stripe_size field to pointerdb proto file #218
Conversation
Pull Request Test Coverage Report for Build 2393
💛 - Coveralls |
protos/pointerdb/pointerdb.proto
Outdated
| @@ -65,12 +65,13 @@ message Pointer { | |||
|
|
|||
| bytes inline_segment = 3; | |||
| RemoteSegment remote = 4; | |||
| int64 size = 5; | |||
| int64 segment_size = 5; | |||
| int32 stripe_size = 6; | |||
There was a problem hiding this comment.
This new field should be part of the RedundancyScheme, not the Pointer.
So there won't be any need to rename the size field to segment_size.
And the new field should not be stripe_size, but erasure_share_size, regardless of what's written in the JIRA ticket. It is the erasure share size that is a parameter of the RS scheme, not the stripe size, although they are related.
navillasa
force-pushed
the
pointer-stripe
branch
from
c359b84
to
bf6dfac
Compare
There was a problem hiding this comment.
Good. We now store the erasure share size in the pointer.
Now we have to make use of it when getting the segment. The Get() method must not use the RedundancyStrategy from the constructor. That one is only for uploading new segments. The Get() method should create a new ErasureScheme from the data we have in the pointer and pass it to the ECClient.
pkg/storage/segments/store.go
Outdated
| @@ -192,7 +193,13 @@ func (s *segmentStore) Get(ctx context.Context, path paths.Path) ( | |||
| return nil, Meta{}, Error.Wrap(err) | |||
| } | |||
|
|
|||
| rr, err = s.ec.Get(ctx, nodes, s.rs, pid, pr.GetSize()) | |||
| es, err := makeErasureScheme(pr.GetRemote().GetRedundancy().GetMinReq(), | |||
| pr.GetRemote().GetRedundancy().GetTotal(), pr.GetRemote().GetRedundancy().GetErasureShareSize()) | |||
There was a problem hiding this comment.
makeEreasureScheme would be even more helpful if it just take a single param pr.GetRemote().GetRedundancy() It can get the min req, total and erasure share size from it in the function implementation.
pkg/storage/segments/store.go
Outdated
| func makeErasureScheme(minReq, total, eShareSize int32) (eestream.ErasureScheme, error) { | ||
| fc, err := infectious.NewFEC(int(minReq), int(total)) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Should we return a wrapped error here? Error.Wrap(err)
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @kaloyan-raev)
pkg/storage/segments/store.go, line 197 at r2 (raw file):
Previously, kaloyan-raev (Kaloyan Raev) wrote…
makeEreasureSchemewould be even more helpful if it just take a single parampr.GetRemote().GetRedundancy()It can get the min req, total and erasure share size from it in the function implementation.
Done.
pkg/storage/segments/store.go, line 216 at r2 (raw file):
Previously, kaloyan-raev (Kaloyan Raev) wrote…
Should we return a wrapped error here?
Error.Wrap(err)
Done.
protos/pointerdb/pointerdb.proto, line 69 at r1 (raw file):
Previously, kaloyan-raev (Kaloyan Raev) wrote…
This new field should be part of the RedundancyScheme, not the Pointer.
So there won't be any need to rename the
sizefield tosegment_size.And the new field should not be
stripe_size, buterasure_share_size, regardless of what's written in the JIRA ticket. It is the erasure share size that is a parameter of the RS scheme, not the stripe size, although they are related.
Done.
* adds erasure_share_size to RS in pointerdb proto * adds makeErasureScheme helper method to segments store * cleans up makeErasureScheme function
Adds
erasure_share_sizefield to the RedundancyScheme message in the pointerdb proto file.Erasure share size is now added to remote pointers in segments store (see makeRemotePointer function).
Adjusts tests in segmentStore accordingly.
This change is