Feat/add big objects support for meta on chain #3063

carpawell · 2024-12-23T21:37:42Z

~~Blocked by #3044.~~

codecov · 2024-12-23T21:40:08Z

Codecov Report

Attention: Patch coverage is 64.10256% with 14 lines in your changes missing coverage. Please review.

Project coverage is 21.96%. Comparing base (7ac9947) to head (8ebd425).
Report is 19 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/services/object/put/distributed.go	0.00%	10 Missing ⚠️
pkg/network/transport/object/grpc/replication.go	66.66%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3063      +/-   ##
==========================================
- Coverage   22.36%   21.96%   -0.40%     
==========================================
  Files         793      797       +4     
  Lines       58713    59852    +1139     
==========================================
+ Hits        13130    13149      +19     
- Misses      44682    45800    +1118     
- Partials      901      903       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Follows nspcc-dev/neofs-node#3063. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

cthulhu-rider · 2024-12-24T07:13:07Z

pkg/network/transport/object/grpc/replication.go

@@ -206,6 +206,12 @@ func objectFromMessage(gMsg *objectGRPC.Object) (*object.Object, error) {
 }

 func (s *Server) metaInfoSignature(o object.Object) ([]byte, error) {
+	firstObj, _ := o.FirstID()
+	if firstObj.IsZero() && o.HasParent() && o.SplitID() == nil {


last operand is inobvious to me. How about create a helper func for this? U check this condition twice here

this differs split V1. in the engine it is required to be possible handle both schemes. in fact, it should be impossible to meet it here, since meta-on-chain containers are new containers, so not V1 is expected but still i would like to see this check

check is OK, i just suggest to share and doc it. Again, if i'd need to check whether object is first or not, i'd miss split ID check

i'd miss split ID check

as i said, this check is the only thing that differs v1 from v2. but if a drop code is requested i usually support it, dropped

cthulhu-rider · 2024-12-24T07:15:27Z

pkg/core/object/replicate.go

@@ -38,14 +38,46 @@ const (
 //	"validuntil": last valid block number for meta information
 //
 // Last valid epoch is object's creation epoch + 10.
-func EncodeReplicationMetaInfo(cID cid.ID, oID, firstPart oid.ID, pSize uint64,
-	deleted, locked []oid.ID, vub uint64, magicNumber uint32) []byte {
+func EncodeReplicationMetaInfo(o object.Object, vub uint64, magicNumber uint32) ([]byte, error) {


callers aint happy about this change. One was completely okay w/o an error, while the second forced to do multiple decoding. If u wanna decrease prm num, typedef a structure

typedef a structure

object.Object is such a struct, why not?

object has no deleted, locked []oid.ID fields

object has no deleted, locked []oid.ID fields

yes, but object.Object is enough to get this information and it will be always gotten like this

this commit was already dropped anyway

cthulhu-rider · 2024-12-24T07:16:27Z

pkg/network/transport/object/grpc/replication.go

+	if err != nil {
+		return nil, fmt.Errorf("encoding metadata")
+	}
+	secondMeta, _ := objectcore.EncodeReplicationMetaInfo(o, secondBlock, s.mNumber)


one more proof of the refactoring dubiousness

this is just a context of this func: you need to repeat the same encoding 3 times, error happens for all or does not happen at all

ok, it is easier to drop this commit

cthulhu-rider · 2024-12-24T07:23:33Z

pkg/core/object/replicate.go

 	deleted, locked []oid.ID, vub uint64, magicNumber uint32) []byte {
 	kvs := []stackitem.MapElement{
 		kv(networkMagicKey, magicNumber),
 		kv(cidKey, cID[:]),
 		kv(oidKey, oID[:]),
+		kv(firstPartKey, firstPart[:]),


i dont suggest to push missing firstPart as zero ID, it's a reserved invalid value. Push emptiness instead: this will match the zero length condition from ur contract change nspcc-dev/neofs-contract@416936f

added optional fields support. firstPart is now one of them

Not sure what made me go this way, there is no reason for it. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

Should have been done in ae6e37d. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

Follows nspcc-dev/neofs-node#3063. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

roman-khimov · 2024-12-26T14:54:43Z

pkg/core/object/replicate.go

 //	"validUntil": last valid block number for meta information
+//	"network": network magic


Better keep them alphabetically sorted (in optional/non-optional groups).

in optional/non-optional groups)

👍

alphabetically sorted

👎. Excessive load to the dev brain "where to place new key?". And i like how they are grouped by type currently

Better keep them alphabetically sorted

well, my suggestion is not a random order, it was an intention. i grouped it like "logically" or as @cthulhu-rider said, currently, it is by type too. do you have any pros as to why it is better to sort them? new value will be harder to add, and alphabet sorting has no advantages (or change my mind)

pkg/core/object/replicate.go

roman-khimov · 2024-12-26T14:57:09Z

pkg/core/object/replicate.go

@@ -59,6 +62,9 @@ func EncodeReplicationMetaInfo(cID cid.ID, oID, firstPart, previousPart oid.ID,
 	if len(locked) > 0 {
 		kvs = append(kvs, oidsKV(lockedKey, locked))
 	}
+	if typ != objectsdk.TypeRegular {
+		kvs = append(kvs, kv(typeKey, uint32(typ)))


Notice that tombstone/lock can be detected by deleted/locked already. But this covers more things, so OK.

you mean lock type with some non-empy deleted should be an error (panic in fact) in this func?

cthulhu-rider · 2024-12-27T10:03:48Z

pkg/core/object/replicate.go

 	validUntilKey   = "validUntil"
+	networkMagicKey = "network"


motivated movement?

the same as in #3063 (comment)

ok-ok, waste of time, more code to review, etc. but i feel better this way

cthulhu-rider · 2024-12-27T10:06:45Z

pkg/core/object/replicate.go

 //	"validUntil": last valid block number for meta information
+//	"network": network magic


in optional/non-optional groups)

👍

alphabetically sorted

👎. Excessive load to the dev brain "where to place new key?". And i like how they are grouped by type currently

cthulhu-rider · 2024-12-27T10:12:32Z

pkg/core/object/replicate.go

+		kv(networkMagicKey, magicNumber),
+	}
+
+	if len(deleted) > 0 {


i'd split unit tests into min and full cases. And include fields added in the following commits

cthulhu-rider · 2024-12-27T10:17:28Z

pkg/core/object/replicate.go

@@ -59,6 +62,9 @@ func EncodeReplicationMetaInfo(cID cid.ID, oID, firstPart, previousPart oid.ID,
 	if len(locked) > 0 {
 		kvs = append(kvs, oidsKV(lockedKey, locked))
 	}
+	if typ != objectsdk.TypeRegular {
+		kvs = append(kvs, kv(typeKey, uint32(typ)))


although typ is uint32, lets to cast to int32. It will be int32 soon (im also unhappy bout this, but wcyd). We'll most likely forget to change conversion later

Map type allows to be flexible and not rely on positional data interpretation, so there is no need to put empty `deleted` or `locked` arrays for objects that do not delete or lock anything. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

pkg/network/transport/object/grpc/replication.go

A first object has its own OID to simplify detection of such objects by indexed meta-information. Closes #2935. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

It is optional since one of the types (REGULAR) is the most expected, and its missing can be recognizable. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

An omission from ae6e37d: check all 3 signatures and slice them correctly. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell self-assigned this Dec 23, 2024

carpawell requested review from roman-khimov and cthulhu-rider as code owners December 23, 2024 21:37

carpawell added a commit to nspcc-dev/neofs-contract that referenced this pull request Dec 23, 2024

container: support metadata with for "big" objects

b840e16

Follows nspcc-dev/neofs-node#3063. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit to nspcc-dev/neofs-contract that referenced this pull request Dec 23, 2024

container/meta: make validuntil camel-cased

5d1731f

Follows nspcc-dev/neofs-node#3063. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit to nspcc-dev/neofs-contract that referenced this pull request Dec 23, 2024

container: support metadata with for "big" objects

416936f

Follows nspcc-dev/neofs-node#3063. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit to nspcc-dev/neofs-contract that referenced this pull request Dec 23, 2024

container/meta: make validuntil camel-cased

5deadec

Follows nspcc-dev/neofs-node#3063. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

cthulhu-rider reviewed Dec 24, 2024

View reviewed changes

carpawell mentioned this pull request Dec 25, 2024

node: init blockchain state on startup #3069

Merged

carpawell changed the title ~~Feat/add first object in meta data~~ Feat/add big objects support for meta on chain Dec 25, 2024

core/object/replicate: make validuntil camel-cased

b194cc2

Not sure what made me go this way, there is no reason for it. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell force-pushed the feat/add-first-object-in-meta-data branch from 3a07bdd to 0265840 Compare December 25, 2024 16:32

pkg/core/object: drop epoch-bases comment to meta encoding func

2bbea7f

Should have been done in ae6e37d. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell force-pushed the feat/add-first-object-in-meta-data branch from 0265840 to 983b38b Compare December 25, 2024 16:42

carpawell requested a review from cthulhu-rider December 25, 2024 16:43

carpawell added a commit to nspcc-dev/neofs-contract that referenced this pull request Dec 25, 2024

container/meta: make validuntil camel-cased

13ef60b

Follows nspcc-dev/neofs-node#3063. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell added a commit to nspcc-dev/neofs-contract that referenced this pull request Dec 25, 2024

container/meta: support metadata with for "big" objects

9be3994

Follows nspcc-dev/neofs-node#3063. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

roman-khimov reviewed Dec 26, 2024

View reviewed changes

cthulhu-rider reviewed Dec 27, 2024

View reviewed changes

node/object/put: allow optional fields

a87f16d

Map type allows to be flexible and not rely on positional data interpretation, so there is no need to put empty `deleted` or `locked` arrays for objects that do not delete or lock anything. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell force-pushed the feat/add-first-object-in-meta-data branch 2 times, most recently from fb4685d to 858354c Compare December 28, 2024 14:52

carpawell requested review from roman-khimov and cthulhu-rider December 28, 2024 14:59

roman-khimov reviewed Dec 28, 2024

View reviewed changes

pkg/network/transport/object/grpc/replication.go Outdated Show resolved Hide resolved

carpawell force-pushed the feat/add-first-object-in-meta-data branch from 858354c to ce1096b Compare December 28, 2024 15:47

carpawell requested a review from roman-khimov December 28, 2024 15:48

roman-khimov approved these changes Dec 28, 2024

View reviewed changes

carpawell force-pushed the feat/add-first-object-in-meta-data branch from ce1096b to fb345bf Compare December 28, 2024 16:30

carpawell force-pushed the feat/add-first-object-in-meta-data branch from fb345bf to cde8332 Compare December 28, 2024 16:33

carpawell requested a review from roman-khimov December 28, 2024 16:33

carpawell added 3 commits December 28, 2024 19:35

node/object/put: push first and previous parts to chain

228d4ac

A first object has its own OID to simplify detection of such objects by indexed meta-information. Closes #2935. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

node/object/put: add object type to meta-on-chain

06e7020

It is optional since one of the types (REGULAR) is the most expected, and its missing can be recognizable. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

node/object/replicate_test: fix replication signature test

8ebd425

An omission from ae6e37d: check all 3 signatures and slice them correctly. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>

carpawell force-pushed the feat/add-first-object-in-meta-data branch from cde8332 to 8ebd425 Compare December 28, 2024 16:36

roman-khimov merged commit b39e7b8 into master Dec 28, 2024
19 of 22 checks passed

roman-khimov deleted the feat/add-first-object-in-meta-data branch December 28, 2024 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/add big objects support for meta on chain #3063

Feat/add big objects support for meta on chain #3063

carpawell commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

cthulhu-rider Dec 24, 2024

carpawell Dec 25, 2024

cthulhu-rider Dec 27, 2024

carpawell Dec 28, 2024 •

edited

Loading

cthulhu-rider Dec 24, 2024

carpawell Dec 25, 2024

cthulhu-rider Dec 27, 2024

carpawell Dec 28, 2024

cthulhu-rider Dec 24, 2024

carpawell Dec 25, 2024

cthulhu-rider Dec 24, 2024

carpawell Dec 25, 2024

roman-khimov Dec 26, 2024

cthulhu-rider Dec 27, 2024

carpawell Dec 28, 2024

roman-khimov Dec 26, 2024

carpawell Dec 28, 2024

cthulhu-rider Dec 27, 2024

carpawell Dec 28, 2024

cthulhu-rider Dec 27, 2024

cthulhu-rider Dec 27, 2024

carpawell Dec 28, 2024

cthulhu-rider Dec 27, 2024

carpawell Dec 28, 2024

		// "validUntil": last valid block number for meta information
		// "network": network magic

Feat/add big objects support for meta on chain #3063

Feat/add big objects support for meta on chain #3063

Conversation

carpawell commented Dec 23, 2024 • edited Loading

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

carpawell Dec 28, 2024 •

edited

Loading