Skip to content

sn: do not respond with epoch in metaheader - #4072

Merged
roman-khimov merged 1 commit into
masterfrom
fix/attach-meta-headers-for-GET-stream-messages
Jul 31, 2026
Merged

sn: do not respond with epoch in metaheader#4072
roman-khimov merged 1 commit into
masterfrom
fix/attach-meta-headers-for-GET-stream-messages

Conversation

@carpawell

@carpawell carpawell commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

This affects only

  1. any REP objects;
  2. EC parts;
  3. EC full object with RANGE.
    This does not affect case implemented in sn/object: Optimize EC GET handling without recovery #3996 (Get service's Prm with WithECTransport() called). Refs GET does not stream meta headers #4071.

@carpawell carpawell self-assigned this Jul 8, 2026
@codecov

codecov Bot commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.20%. Comparing base (df6385a) to head (ed4da94).

Files with missing lines Patch % Lines
cmd/neofs-node/accounting.go 0.00% 1 Missing ⚠️
cmd/neofs-node/session.go 0.00% 1 Missing ⚠️
pkg/services/accounting/server.go 0.00% 1 Missing ⚠️
pkg/services/session/server.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4072      +/-   ##
==========================================
- Coverage   27.20%   27.20%   -0.01%     
==========================================
  Files         676      676              
  Lines       46241    46227      -14     
==========================================
- Hits        12582    12577       -5     
+ Misses      32445    32437       -8     
+ Partials     1214     1213       -1

☔ View full report in Codecov by Harness.
📢 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.

@carpawell carpawell changed the title sn: attach meta header to GET's streams sn: attach meta header to GET streams Jul 8, 2026
roman-khimov
roman-khimov previously approved these changes Jul 8, 2026
View reviewed changes

@roman-khimov roman-khimov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to tell about in chlog?

@carpawell

carpawell commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

PR adds a metaheader to every message. This is how i read the spec currently. But i think @cthulhu-rider thinks differently. Once we decide i will update chlog.

@roman-khimov
roman-khimov dismissed their stale review July 9, 2026 20:12

Let's settle the API thing first

@carpawell
carpawell force-pushed the fix/attach-meta-headers-for-GET-stream-messages branch from ec3c5a5 to 6383ead Compare July 27, 2026 15:43
@carpawell carpawell changed the title sn: attach meta header to GET streams sn: do not respond with epoch in metaheader Jul 27, 2026
@carpawell

carpawell commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Attaching version is OK to me now.

But if we want to "Must be attached only if server's version is lower than client's one" then it will require more adoption: current code is just a number of helper functions that always attach the version; there is no local request in these funcs.

@carpawell
carpawell requested a review from roman-khimov July 27, 2026 15:50
cthulhu-rider
cthulhu-rider reviewed Jul 27, 2026
View reviewed changes

@cthulhu-rider cthulhu-rider left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version behavior does not yet correspond to nspcc-dev/neofs-api#404

Comment thread pkg/services/session/server.go
Body: body,
MetaHeader: &protosession.ResponseMetaHeader{
Version: version.Current().ProtoMessage(),
Epoch: s.net.CurrentEpoch(),

@cthulhu-rider cthulhu-rider Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes s.net unused so it's worth to be deparameterized

pls review other cases. Same can be done or at least narrow the dependency interface

@carpawell carpawell Jul 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped when unused

Comment thread pkg/services/object/proto.go

buf[0] = iprotobuf.TagBytes2
off := 1 + binary.PutUvarint(buf[1:], uint64(ln))

@cthulhu-rider cthulhu-rider Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separately, although it is hardly necessary

@carpawell carpawell Jul 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did not understand, what exactly is unnecessary?

@carpawell
carpawell force-pushed the fix/attach-meta-headers-for-GET-stream-messages branch from 6383ead to fe2a73f Compare July 31, 2026 09:43
@carpawell

carpawell commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Version behavior does not yet correspond to nspcc-dev/neofs-api#404

As i said, i would leave it as is, since it is "optional" in API, so attaching it always is ok to me. But branched returning version.

@carpawell
carpawell requested a review from roman-khimov July 31, 2026 09:48
@carpawell
carpawell force-pushed the fix/attach-meta-headers-for-GET-stream-messages branch from fe2a73f to 0cd51c3 Compare July 31, 2026 09:59
@carpawell

carpawell commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

Left #4110. Only epoch drop for now.

@carpawell
sn: do not respond with epoch in ResponseMetaHeader
ed4da94 
Refs nspcc-dev/neofs-api#404.

Signed-off-by: Pavel Karpy <carpawell@nspcc.io>
@carpawell
carpawell force-pushed the fix/attach-meta-headers-for-GET-stream-messages branch from 0cd51c3 to ed4da94 Compare July 31, 2026 10:01
@roman-khimov roman-khimov added this to the v0.55.0 milestone Jul 31, 2026
@roman-khimov
roman-khimov merged commit fc9f062 into master Jul 31, 2026
21 of 22 checks passed
@roman-khimov
roman-khimov deleted the fix/attach-meta-headers-for-GET-stream-messages branch July 31, 2026 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cthulhu-rider cthulhu-rider cthulhu-rider left review comments
@roman-khimov roman-khimov roman-khimov approved these changes
@End-rey End-rey Awaiting requested review from End-rey End-rey is a code owner

Assignees

@carpawell carpawell

Labels

None yet

Projects

None yet

Milestone

v0.55.0

Development

Successfully merging this pull request may close these issues.

3 participants

@carpawell @roman-khimov @cthulhu-rider