EC put #1064

Merged
fyrchik merged 5 commits from dstepanov-yadro/frostfs-node:feat/ec_put into master 2024-04-09 07:08:55 +00:00
  • put unprepared objects to EC container
  • put prepared objects to EC container is not supported because frostfs-node requires user's private key for EC chunks
  • policer skips EC containers
  • frostfs-cli outputs EC header
  • added VSCode 4 storage nodes + IR configuration
- put unprepared objects to EC container - put prepared objects to EC container is not supported because frostfs-node requires user's private key for EC chunks - policer skips EC containers - frostfs-cli outputs EC header - added VSCode `4 storage nodes + IR` configuration
dstepanov-yadro force-pushed feat/ec_put from b9fdf9afd0 to 553e43a37e 2024-03-29 13:52:02 +00:00 Compare
dstepanov-yadro force-pushed feat/ec_put from 553e43a37e to 8b59030e08 2024-03-29 14:00:40 +00:00 Compare
dstepanov-yadro force-pushed feat/ec_put from 8b59030e08 to 3d61db3986 2024-04-01 09:29:09 +00:00 Compare
dstepanov-yadro force-pushed feat/ec_put from 3d61db3986 to 47434d621c 2024-04-01 09:30:53 +00:00 Compare
dstepanov-yadro force-pushed feat/ec_put from 47434d621c to 4eac25598f 2024-04-01 12:19:24 +00:00 Compare
dstepanov-yadro force-pushed feat/ec_put from 4eac25598f to a83dfa4f01 2024-04-01 12:26:55 +00:00 Compare
dstepanov-yadro force-pushed feat/ec_put from a83dfa4f01 to 1ded16ab9a 2024-04-01 12:27:35 +00:00 Compare
dstepanov-yadro force-pushed feat/ec_put from 1ded16ab9a to da3a157104 2024-04-01 12:37:09 +00:00 Compare
dstepanov-yadro force-pushed feat/ec_put from da3a157104 to 98350bad84 2024-04-01 12:41:38 +00:00 Compare
dstepanov-yadro changed title from WIP: EC put to EC put 2024-04-01 12:45:03 +00:00
dstepanov-yadro requested review from storage-core-committers 2024-04-01 12:47:18 +00:00
dstepanov-yadro requested review from storage-core-developers 2024-04-01 12:49:17 +00:00
fyrchik requested changes 2024-04-01 13:22:48 +00:00
@ -58,11 +58,13 @@ It will be stored in sidechain when inner ring will accepts it.`,
"use --force option to skip this check: %w", err)
for i, nodes := range nodesByRep {
//lint:ignore SA1019 will be fixed later
Owner

Do we have a task or will it be done in this PR?

Do we have a task or will it be done in this PR?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -0,0 +45,4 @@
}
if !placement.IsECSupported(obj) {
// must be resolved by caller
Owner

What do you mean by this comment?

What do you mean by this comment?
Author
Member

ec_writer supports only regular objects. Linking objects, tombstones and locks write should be handled by defaultWriter (aka repWriter).
The current implementation satisfies this condition. It's just an additional check and foolproof.

`ec_writer` supports only regular objects. Linking objects, tombstones and locks write should be handled by `defaultWriter` (aka `repWriter`). The current implementation satisfies this condition. It's just an additional check and foolproof.
fyrchik marked this conversation as resolved
@ -0,0 +211,4 @@
// start node from according part index
// but try others if it fails
var err error
for i := 0; i < len(nodes); i++ {
Owner

We can't try all others, because this violates the assumption of how many nodes can fail. With this code it could happen that all chunks reside on one node.

I suggest to fail here: we already fail in a similar way for big objects (so there could be garbage).
Later we can improve this (probably i < len(nodes) should be replaced with something else, but not i < parityCount, so this is not obvious).

We can't try _all_ others, because this violates the assumption of how many nodes can fail. With this code it could happen that all chunks reside on one node. I suggest to fail here: we already fail in a similar way for big objects (so there could be garbage). Later we can improve this (probably `i < len(nodes)` should be replaced with something else, but not `i < parityCount`, so this is not obvious).
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -247,0 +258,4 @@
return &multiObjectWriter{
ecWriter: &ecWriter{
cfg: p.cfg,
placementOpts: append(prm.traverseOpts, placement.WithCopyNumbers(nil)), // copies number ignored for EC
Owner

What happens if I have EC 2+1 and 4 nodes in the container?

What happens if I have `EC 2+1` and 4 nodes in the container?
Author
Member

What exactly are you asking? EC chunks will be saved on 3 of 4 container nodes.

What exactly are you asking? EC chunks will be saved on 3 of 4 container nodes.
@ -0,0 +7,4 @@
)
// IsECPlacement returns True if policy is erasure coding policy.
func IsECPlacement(policy netmapSDK.PlacementPolicy) bool {
Owner

These functions don't seem to be related to placement (and thus to this package) (especially ECDataCount which seems a rather narrow-scoped helper), why are they here?

These functions don't seem to be related to placement (and thus to this package) (especially `ECDataCount` which seems a rather narrow-scoped helper), why are they here?
Author
Member

I don't think these functions don't seem to be related to placement. I think there are about placement.

I don't think these functions don't seem to be related to placement. I think there are about placement.
Author
Member

moved to core

moved to `core`
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed feat/ec_put from 98350bad84 to f17686fc29 2024-04-01 14:11:24 +00:00 Compare
aarifullin reviewed 2024-04-01 15:44:16 +00:00
@ -0,0 +12,4 @@
type multiObjectWriter struct {
ecWriter transformer.ObjectWriter
repWriter transformer.ObjectWriter
Member
  1. I don't see any reason to keep repWriter within multiObjectWriter at least because you don't use newECWriter if container does not support EC policy. So, it seems it always skips m.repWriter.WriteObject(ctx, obj). Please, fix me, if I am incorrect

  2. multi- prefix sounds like the object writer is able to choose how to write an object to a container: either by EC placement or by REP although a container adheres to single placement. What do you think about the idea to use objectWriterDispatcher name instead?

1. I don't see any reason to keep `repWriter` within `multiObjectWriter` at least because you don't use `newECWriter` if container does not support EC policy. So, it seems it always skips `m.repWriter.WriteObject(ctx, obj)`. Please, fix me, if I am incorrect 2. `multi-` prefix sounds like the object writer is able to *choose* how to write an object to a container: either by `EC` placement or by `REP` although a container adheres to single placement. What do you think about the idea to use `objectWriterDispatcher` name instead?
Author
Member
  1. EC container stores some objects (tombstones, locks, linking objects) as regular objects: these objects must be saved on every container node without EC splitting.
  2. done
1. EC container stores some objects (tombstones, locks, linking objects) as regular objects: these objects must be saved on every container node without EC splitting. 2. done
Member

EC container stores some objects (tombstones, locks, linking objects) as regular objects: these objects must be saved on every container node without EC splitting.

Thanks for explanation

UPD: btw, I incorrectly read your code

if container.IsECContainer(prm.cnr) && object.IsECSupported(prm.hdr)

I didn't notice you pass header to IsECSupported - that is why I've got confused. But for now it is OK 👍

> EC container stores some objects (tombstones, locks, linking objects) as regular objects: these objects must be saved on every container node without EC splitting. Thanks for explanation UPD: btw, I incorrectly read your code ```go if container.IsECContainer(prm.cnr) && object.IsECSupported(prm.hdr) ``` I didn't notice you pass header to `IsECSupported` - that is why I've got confused. But for now it is OK 👍
aarifullin marked this conversation as resolved
dstepanov-yadro force-pushed feat/ec_put from f17686fc29 to 7d1ce328cf 2024-04-01 15:55:08 +00:00 Compare
aarifullin approved these changes 2024-04-01 17:57:53 +00:00
acid-ant approved these changes 2024-04-03 11:39:53 +00:00
dstepanov-yadro force-pushed feat/ec_put from 7d1ce328cf to 04a23efef6 2024-04-03 12:35:27 +00:00 Compare
Owner

image

![image](/attachments/a0e97ea9-955f-4ead-96b6-550797d3c40d)
fyrchik added this to the v0.39.0 milestone 2024-04-09 07:04:00 +00:00
fyrchik approved these changes 2024-04-09 07:08:13 +00:00
fyrchik merged commit f5b67c6735 into master 2024-04-09 07:08:55 +00:00
fyrchik referenced this pull request from a commit 2024-04-09 07:08:57 +00:00
Sign in to join this conversation.
No reviewers
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-node#1064
No description provided.