sn/object: Optimize EC GET handling without recovery#3996
Open
cthulhu-rider wants to merge 1 commit into
Open
Conversation
| keys: keyStorage, | ||
| } | ||
| server := objectService.New(objSvc, mNumber, c.cfgObject.pool.search, fsChain, storage, c.metaService, c.key.PrivateKey, c.metricsCollector, aclChecker, aclSvc, coreConstructor) | ||
| server := objectService.New(objSvc, mNumber, c.cfgObject.pool.search, fsChain, storage, c.metaService, c.key.PrivateKey, c.metricsCollector, aclChecker, aclSvc, coreConstructor, c.log) |
Contributor
Author
There was a problem hiding this comment.
no, i did not touch those TODOs
| // | ||
| // If next endpoint is unavailable or f returns [ErrSkipConnection] for it, | ||
| // ForAnyGRPCConn continues. If this happens on all endpoints, ForAnyGRPCConn | ||
| // returns [ErrAllConnectionsSkipped]. |
Member
There was a problem hiding this comment.
This change can be separated from a single big commit.
| if errors.Is(err, ctx.Err()) { | ||
| return false, 0, 0, 0, err | ||
| } | ||
| // TODO: if error is due to incorrect request, error should be returned. How to catch this? |
Member
There was a problem hiding this comment.
Related to #3413, container node checks the original request then supposedly it always creates perfect new ones (so that any error in that requests is kinda an internal error to client).
| if chunkFrom > 0 { | ||
| _, ok := fld.MoveNext(chunkFrom) | ||
| if !ok { | ||
| return 0, fmt.Errorf("%w while moving to offset=%d in chunk with length=%d", io.ErrUnexpectedEOF, chunkFrom, chunkLen) |
| var ok bool | ||
| fld, ok = fld.MoveNext(chunkTo - chunkFrom) | ||
| if !ok { | ||
| return 0, fmt.Errorf("%w while moving to offset=%d in chunk with length=%d", io.ErrUnexpectedEOF, chunkTo-chunkFrom, chunkLen-chunkFrom) |
| } | ||
|
|
||
| if controlCh != nil { | ||
| if controlCh != nil { |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes EC GET handling by attempting to stream EC parts directly to the client before falling back to full EC recovery, reducing latency and intermediate buffering.
Changes:
- Adds EC GET transport logic for local/remote EC part streaming and range forwarding.
- Adds low-level protobuf helpers for request/response construction and object header parsing.
- Updates multi-address client retry semantics to support explicitly skipped connections.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/services/object/util.go |
Extracts split-info parsing and adds partial-copy read error metadata. |
pkg/services/object/server.go |
Wires EC transport into GET and generalizes stream-copy helpers. |
pkg/services/object/proto.go |
Adds fixed-length protobuf/signature helpers for manual request construction. |
pkg/services/object/get/util.go |
Adds partial EC copy error type. |
pkg/services/object/get/service.go |
Defines EC transport interface and shared stream-control errors. |
pkg/services/object/get/prm.go |
Adds EC transport parameter to GET parameters. |
pkg/services/object/get/get.go |
Passes EC transport into EC object copy path. |
pkg/services/object/get/exec.go |
Uses exported stream failure sentinel. |
pkg/services/object/get/ec.go |
Adds direct EC part streaming orchestration before recovery fallback. |
pkg/services/object/get.go |
Implements EC part request creation and remote/local streaming transport. |
pkg/network/cache/clients.go |
Adds skip-connection handling and structured multi-endpoint errors. |
pkg/network/cache/clients_internal_test.go |
Tests multi-endpoint error wrapping behavior. |
pkg/core/client/client.go |
Adds connection-skip sentinel errors to the multi-address client API. |
internal/protobuf/tags.go |
Adds protobuf byte tag constants for fields 7–9. |
internal/protobuf/tags_test.go |
Extends tag constant validation. |
internal/protobuf/seekers.go |
Adds enum field lookup helper. |
internal/protobuf/api.go |
Adds fixed protobuf lengths for container IDs and object addresses. |
internal/protobuf/api_test.go |
Tests fixed protobuf message length constants. |
internal/object/wire.go |
Adds object header helpers for parent fields, payload length, and type. |
cmd/neofs-node/object.go |
Passes logger into object service construction. |
CHANGELOG.md |
Records EC GET optimization in changelog. |
Comments suppressed due to low confidence (4)
pkg/services/object/get.go:842
- This
breakexits only theswitch, not the surrounding receive loop. After the requested range is copied, the code keeps reading the rest of the GET stream and can forward extra empty chunks; returncopiedor use a labeled break here.
if copied == ln {
break
pkg/services/object/get.go:1080
handleRangeResponseBodyOneofreturnsFieldRangeResponseBodySplitInfo, so thisFieldGetResponseBodySplitInfocase is never reached for RANGE split-info responses. Use the RANGE split-info constant; otherwise split-info responses fall into the default unsupported-oneof error path.
case protoobject.FieldGetResponseBodySplitInfo:
respBuf.Free()
if !first {
return 0, errors.New("received split info in non-first message")
}
return 0, errors.New("unexpected split info status response")
pkg/services/object/get.go:587
- The retry for a partially copied first EC part requests
parentPldLen-copiedPartPldbytes from part #0, but the range length must be limited to the remaining bytes in that part (partPldLen-copiedPartPld). For multi-part EC objects this can request past the part boundary and produce an out-of-range response after data has already been streamed.
copiedFromNode, err := x.copyRemotePartRange(ctx, conn, ruleIdx, partIdx, copiedPartPld, parentPldLen-copiedPartPld, nil)
pkg/services/object/get/ec.go:1547
- The local retry path has the same boundary issue: it requests
parentPldLen-copiedPartPldbytes from a single EC part instead of the remaining part length. This can overrun the part payload for objects split across multiple data parts; usepartPldLen-copiedPartPldhere.
copiedFromNode, err = transport.CopyLocalECPartRange(ctx, s.localObjects.(*engine.StorageEngine), ruleIdx, 0, copiedPartPld, parentPldLen-copiedPartPld, nil)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cthulhu-rider
force-pushed
the
ec-get
branch
2 times, most recently
from
4b1ce3e to
c899158
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3996 +/- ##
==========================================
- Coverage 28.33% 27.80% -0.54%
==========================================
Files 681 681
Lines 45753 46760 +1007
==========================================
+ Hits 12964 13001 +37
- Misses 31556 32520 +964
- Partials 1233 1239 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cthulhu-rider
force-pushed
the
ec-get
branch
3 times, most recently
from
183f040 to
513ee29
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3952.