Fix/meta/full index #3177

carpawell · 2025-02-26T01:52:40Z

No description provided.

codecov · 2025-02-26T14:34:58Z

Codecov Report

Attention: Patch coverage is 68.60465% with 108 lines in your changes missing coverage. Please review.

Project coverage is 23.42%. Comparing base (ea6b99c) to head (1c2d1a6).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/services/meta/containers.go	80.08%	31 Missing and 15 partials ⚠️
pkg/core/object/metadata.go	0.00%	22 Missing ⚠️
cmd/neofs-node/meta.go	0.00%	21 Missing ⚠️
pkg/services/meta/blocks.go	77.50%	6 Missing and 3 partials ⚠️
pkg/local_object_storage/metabase/metadata.go	85.00%	2 Missing and 1 partial ⚠️
pkg/local_object_storage/metabase/version.go	25.00%	0 Missing and 3 partials ⚠️
pkg/services/meta/meta.go	0.00%	2 Missing ⚠️
pkg/local_object_storage/metabase/put.go	50.00%	0 Missing and 1 partial ⚠️
pkg/services/meta/notifications.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3177      +/-   ##
==========================================
+ Coverage   23.22%   23.42%   +0.20%     
==========================================
  Files         760      760              
  Lines       60633    60848     +215     
==========================================
+ Hits        14080    14254     +174     
- Misses      45558    45587      +29     
- Partials      995     1007      +12

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pkg/services/meta/blocks.go

pkg/services/meta/containers.go

cthulhu-rider · 2025-02-27T08:03:53Z

pkg/services/meta/blocks.go

+		wg.Go(func() error {
+			err := m.handleObjectNotification(ctx, s, ev)
+			if err != nil {
+				return fmt.Errorf("handling %s/%s object notification: %w", ev.cID, ev.oID, err)


previously we logged all errors, now only the earliest one. I tink they all deserve attention, so suggest to keep logging here, and ignore on Wait()

will fix depening of what we will decide with @roman-khimov about object handing order in the thread above

cthulhu-rider · 2025-02-27T08:11:27Z

pkg/services/meta/containers.go

 	s.m.Lock()
 	defer s.m.Unlock()

-	newKVs := make(map[string][]byte)
+	dbKVs := make(map[string][]byte)
+	mptKVs := make(map[string][]byte)


we can add its els directly to s.opsBatch, right?

we can but dbKVs and commonKVs are still required so imo it is ok to keep them the same. this map should be like ~10 fields long, i do not thinks it even escaped. also it consists of string which made this change almost useless

but ok

cthulhu-rider · 2025-02-27T08:14:02Z

pkg/services/meta/containers.go

+		commonKVs[string(append([]byte{0, deletedIndex}, commsuffix...))] = e.deletedObjects
+		clearMpt, clearDB := deleteObjectsOps(s.db, e.deletedObjects)
+		maps.Copy(mptKVs, clearMpt)
+		maps.Copy(dbKVs, clearDB)


these maps can be accepted and set by deleteObjectsOps so we dont need to copy them

makes sence with latest changes

cthulhu-rider · 2025-02-27T08:16:57Z

pkg/services/meta/containers.go

+	phy := hasParent || (fPart.IsZero() && parID.IsZero())
+	pldHash, ok := h.PayloadChecksum()
+	if !ok {
+		return errors.New("missing payload checksum")


i'd handle all error cases on the func top

get object's fields in a sep func to pass many args to this func? how it helps?

it's a common practice to check for return errors first in a function. I dont suggest to change its signature

i was inspired by your func, it was ctrl-c ctrl-v. ok, reused your new verification func

cthulhu-rider · 2025-02-27T08:27:30Z

cmd/neofs-node/meta.go

+
+	var hPrm getsvc.HeadPrm
+	hPrm.SetHeaderWriter(&hw)
+	hPrm.WithAddress(addr)


Suggested change

hPrm.WithAddress(addr)

hPrm.WithAddress(oid.NewAddress(cID, oID))

cthulhu-rider · 2025-02-27T08:57:04Z

pkg/services/meta/containers.go

+
+				resDB[string(keyToDrop)] = nil
+
+				if vInt, isInt := parseInt(string(attrV)); isInt {


again can panic

cthulhu-rider · 2025-02-27T09:00:15Z

pkg/services/meta/containers.go

+					keyToDrop = append(keyToDrop, attrToIntIndex)
+					keyToDrop = append(keyToDrop, attributeDelimiter...)
+					keyToDrop = keyToDrop[:len(keyToDrop)+intValLen]
+					putBigInt(keyToDrop, vInt)


wrong place to put

cthulhu-rider · 2025-02-27T09:03:09Z

pkg/services/meta/meta.go

 	// List returns node's containers that support chain-based meta data and
 	// any error that does not allow listing.
 	List() (map[cid.ID]struct{}, error)
 	// IsMineWithMeta checks if the given CID has meta enabled and current
 	// node belongs to it.
 	IsMineWithMeta(cid.ID) (bool, error)
+	// Head returns actual object header from the NeoFS network (non-local
+	// objects should also be returned). Missing, removed object statuses
+	// must be reported according to API statuses from SDK.


this statement is not needed actually, Meta does not care what error is returned

whay do you mean? an object can be removed, not found, etc. i can imagine future status logic based on response

imaginary logic makes the interface heavier without any practical benefit

we do need to have a reaction on missing objects, it is already planned logic, not imaginary

pkg/services/meta/meta.go

cthulhu-rider · 2025-02-27T09:06:36Z

pkg/services/meta/notifications.go

-	err := s.putObject(e)
+	h, err := m.net.Head(ctx, e.cID, e.oID)
+	if err != nil {
+		return fmt.Errorf("HEAD rpc error: %w", err)


there is no guarantee that the RPC has been actually done - some preliminary errors are possible, or only local execution. Simply

Suggested change

return fmt.Errorf("HEAD rpc error: %w", err)

return fmt.Errorf("HEAD object: %w", err)

well, changed but why?

RPC is a remote communication, here we dont always go to remote

carpawell · 2025-03-01T01:47:13Z

https://rest.fs.neo.org/HXSaMJXk2g8C14ht8HSi7BBaiYZ1HeWh2xnWPGQCg4H6/2664-1740774980/index.html

pkg/services/meta/blocks.go

roman-khimov · 2025-03-05T12:46:34Z

pkg/services/meta/containers.go

+	putIntAttribute(batch, id, objectsdk.FilterCreationEpoch, strconv.FormatUint(creationEpoch, 10), new(big.Int).SetUint64(creationEpoch))
+	putIntAttribute(batch, id, objectsdk.FilterPayloadSize, strconv.FormatUint(pSize, 10), new(big.Int).SetUint64(pSize))
+	putPlainAttribute(batch, id, objectsdk.FilterPayloadChecksum, string(pldHash.Value()))
+	putPlainAttribute(batch, id, objectsdk.FilterPayloadHomomorphicHash, string(pldHmmHash))


In many cases that can be a no-op (conversion) in compiled code.

roman-khimov · 2025-03-05T12:51:28Z

pkg/services/meta/containers.go

+			case oidToAttrIndex:
+				i := bytes.Index(k, attributeDelimiter)
+				if i < 0 {
+					panic(fmt.Errorf("unexpected attribute index without delimeter: %s", string(k)))


panic is DOS. We can complain (loudly with "your DB is broken"), but we better keep going.

roman-khimov · 2025-03-06T19:12:40Z

Conflicts.

Make underlying KV database store the same index structure as in the new metabase structure for search V2. Exclude repetitive fields from the database in the old MPT form. Receive object headers via internal object get (head) service. Closes #3139. Signed-off-by: Pavel Karpy <[email protected]>

This functionality will be shared for some time meta-service feature is being developed. Refs #3139. Signed-off-by: Pavel Karpy <[email protected]>

Refs #3139. Signed-off-by: Pavel Karpy <[email protected]>

It is the same for metabase and metaservice. Index delimiter was also moved. Signed-off-by: Pavel Karpy <[email protected]>

carpawell self-assigned this Feb 26, 2025

carpawell force-pushed the fix/meta/full-index branch from cb36362 to 981f5cf Compare February 26, 2025 14:32

carpawell marked this pull request as ready for review February 26, 2025 14:41

carpawell requested review from roman-khimov and cthulhu-rider as code owners February 26, 2025 14:41

roman-khimov reviewed Feb 26, 2025

View reviewed changes

pkg/services/meta/blocks.go Outdated Show resolved Hide resolved

pkg/services/meta/containers.go Outdated Show resolved Hide resolved

carpawell force-pushed the fix/meta/full-index branch from 981f5cf to 3299a41 Compare February 26, 2025 20:37

carpawell mentioned this pull request Feb 26, 2025

Validate (refuse) events from chain by metaservice based on current state #3178

Closed

carpawell force-pushed the fix/meta/full-index branch from 3299a41 to 5088635 Compare February 26, 2025 20:43

carpawell requested a review from roman-khimov February 26, 2025 20:48

cthulhu-rider reviewed Feb 27, 2025

View reviewed changes

carpawell force-pushed the fix/meta/full-index branch 2 times, most recently from 9943362 to f7ebf89 Compare February 27, 2025 14:49

carpawell marked this pull request as draft February 27, 2025 17:09

carpawell force-pushed the fix/meta/full-index branch from 5d22263 to a39907e Compare February 28, 2025 19:07

carpawell requested a review from cthulhu-rider March 1, 2025 01:46

carpawell marked this pull request as ready for review March 1, 2025 01:46

carpawell force-pushed the fix/meta/full-index branch from a39907e to 82a66e1 Compare March 4, 2025 11:30

roman-khimov reviewed Mar 5, 2025

View reviewed changes

carpawell force-pushed the fix/meta/full-index branch from 82a66e1 to 6652296 Compare March 5, 2025 15:37

carpawell requested a review from roman-khimov March 5, 2025 15:42

carpawell force-pushed the fix/meta/full-index branch 3 times, most recently from fcf8422 to e9642ae Compare March 6, 2025 09:49

carpawell force-pushed the fix/meta/full-index branch from e9642ae to a6fc57d Compare March 7, 2025 08:42

carpawell added 2 commits March 7, 2025 12:01

metabase: export PutMetadataForObject

da0d91f

This functionality will be shared for some time meta-service feature is being developed. Refs #3139. Signed-off-by: Pavel Karpy <[email protected]>

carpawell added 2 commits March 7, 2025 12:01

node/meta: add meta indexes compatibility test

5189bdf

Refs #3139. Signed-off-by: Pavel Karpy <[email protected]>

node/meta: deduplicate header verification

1c2d1a6

It is the same for metabase and metaservice. Index delimiter was also moved. Signed-off-by: Pavel Karpy <[email protected]>

carpawell force-pushed the fix/meta/full-index branch from a6fc57d to 1c2d1a6 Compare March 7, 2025 09:02

roman-khimov approved these changes Mar 7, 2025

View reviewed changes

roman-khimov merged commit 5a9e733 into master Mar 7, 2025
19 of 22 checks passed

roman-khimov deleted the fix/meta/full-index branch March 7, 2025 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/meta/full index #3177

Fix/meta/full index #3177

carpawell commented Feb 26, 2025

codecov bot commented Feb 26, 2025 •

edited

Loading

cthulhu-rider Feb 27, 2025

carpawell Feb 27, 2025 •

edited

Loading

cthulhu-rider Feb 27, 2025

carpawell Feb 27, 2025

cthulhu-rider Feb 27, 2025

carpawell Feb 27, 2025

cthulhu-rider Feb 27, 2025

carpawell Feb 27, 2025

cthulhu-rider Mar 5, 2025

carpawell Mar 6, 2025

cthulhu-rider Feb 27, 2025

cthulhu-rider Feb 27, 2025

cthulhu-rider Feb 27, 2025

cthulhu-rider Feb 27, 2025

carpawell Feb 27, 2025 •

edited

Loading

cthulhu-rider Mar 5, 2025

carpawell Mar 6, 2025

cthulhu-rider Feb 27, 2025

carpawell Feb 27, 2025

cthulhu-rider Mar 5, 2025

carpawell commented Mar 1, 2025

roman-khimov Mar 5, 2025

roman-khimov Mar 5, 2025

roman-khimov commented Mar 6, 2025

	hPrm.WithAddress(addr)
	hPrm.WithAddress(oid.NewAddress(cID, oID))


		resDB[string(keyToDrop)] = nil

		if vInt, isInt := parseInt(string(attrV)); isInt {

	return fmt.Errorf("HEAD rpc error: %w", err)
	return fmt.Errorf("HEAD object: %w", err)

Fix/meta/full index #3177

Fix/meta/full index #3177

Conversation

carpawell commented Feb 26, 2025

codecov bot commented Feb 26, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

carpawell Feb 27, 2025 • 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

carpawell Feb 27, 2025 • 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

carpawell commented Mar 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-khimov commented Mar 6, 2025

codecov bot commented Feb 26, 2025 •

edited

Loading

carpawell Feb 27, 2025 •

edited

Loading

carpawell Feb 27, 2025 •

edited

Loading