Skip to content

feat(sdk): consume the request counter for partition operations - #3958

Open
numinnex wants to merge 4 commits into
masterfrom
fix_sdk_request_id_for_partition
Open

feat(sdk): consume the request counter for partition operations#3958
numinnex wants to merge 4 commits into
masterfrom
fix_sdk_request_id_for_partition

Conversation

@numinnex

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Aug 24, 2026
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.58333% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.54%. Comparing base (9c6451d) to head (599b341).

Files with missing lines Patch % Lines
core/consensus/src/client_table.rs 85.71% 1 Missing and 1 partial ⚠️
...he/iggy/client/async/tcp/vsr/ConsensusSession.java 0.00% 1 Missing and 1 partial ⚠️
foreign/go/internal/vsr/envelope.go 0.00% 0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3958      +/-   ##
============================================
- Coverage     84.20%   80.54%   -3.66%     
+ Complexity     1398     1395       -3     
============================================
  Files          1219     1219              
  Lines        173642   159656   -13986     
  Branches     140740   126772   -13968     
============================================
- Hits         146208   128602   -17606     
- Misses        23468    26920    +3452     
- Partials       3966     4134     +168     
Components Coverage Δ
Rust Core 80.43% <94.59%> (-4.58%) ⬇️
Java SDK 67.18% <0.00%> (-0.05%) ⬇️
C# SDK 75.31% <ø> (-0.08%) ⬇️
Python SDK 90.06% <ø> (ø)
PHP SDK 85.65% <ø> (ø)
Node SDK 96.06% <100.00%> (-0.01%) ⬇️
Go SDK 69.07% <0.00%> (-0.02%) ⬇️
Files with missing lines Coverage Δ
core/sdk/src/quic/quic_client.rs 75.26% <ø> (ø)
core/sdk/src/vsr.rs 94.40% <100.00%> (+0.49%) ⬆️
foreign/csharp/Iggy_SDK/Vsr/ConsensusSession.cs 98.96% <ø> (-0.04%) ⬇️
foreign/csharp/Iggy_SDK/Vsr/VsrOperation.cs 95.83% <ø> (-0.11%) ⬇️
foreign/go/internal/vsr/operation.go 100.00% <ø> (ø)
foreign/go/internal/vsr/session.go 100.00% <ø> (ø)
...apache/iggy/client/async/tcp/vsr/VsrOperation.java 91.83% <ø> (-0.17%) ⬇️
...e/iggy/client/async/tcp/vsr/VsrRequestEncoder.java 94.87% <ø> (-0.37%) ⬇️
foreign/node/src/wire/vsr/index.ts 100.00% <100.00%> (ø)
foreign/node/src/wire/vsr/operation.ts 100.00% <ø> (ø)
... and 4 more

... and 210 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@numinnex

Copy link
Copy Markdown
Contributor Author

Merge after #3945

@hubcio hubcio 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.

the id half looks right and lands uniformly in all five sdks. the checksum half doesn't belong in the same pr. inline comments cover what's inside the diff - the rest sits outside it:

core/server/src/dispatch.rs:1594-1604 - the partition path overwrites new_header.client with transport_client_id and clamps request to max(1). so the client half of a (client, request) dedup key is already discarded before the partition plane sees it, and transport_client_id changes on the very reconnect a retry follows. minting distinct ids buys a future dedup layer nothing until that rewrite changes too. the comment sitting right there - "a zero request id (the SDK does not number data-plane ops) is normalized" - is falsified by this pr and should be updated in it.

core/sdk/src/tcp/tcp_client.rs:1266 - the new hash runs inside the consensus_session std mutex. the comment at :121-124 picks std over tokio precisely because that section is "pure CPU"; it's now a full-batch hash blocking a tokio worker. quic_client.rs:630 is the same shape. hash the payload before taking the lock.

foreign/java/java-sdk/src/main/java/org/apache/iggy/client/async/tcp/vsr/ConsensusSession.java:56 - beginRegister() resets requestCounter to 1 but leaves correlationCounter alone. partition ops now draw from the counter that resets, so a re-login with an in-flight send can re-mint a RequestKey already in pendingRequests; registerRequest throws and writeVsrFrame turns that into closeChannel. that window used to be metadata-only.

core/simulator/src/client.rs:121-144 - still models the pre-pr client: partition ids from a separate counter at 1 << 63, metadata ids contiguous, checksum zero. the one tool that could find this interaction can't generate the pattern any more. its doc at :64 and :124-125 also still claims a gap-free requirement and a RequestGap, which client_table.rs:525 says does not exist.

stale doc sites: core/binary_protocol/src/consensus/header.rs:290 says the wire sends zero, client_table.rs:581 and :2825 repeat it, and node header.ts:116, go header.go:119, csharp VsrHeader.cs:24, java VsrHeaders.java:29 all state the server does not read the field. six places, all false. fix header.rs first since the others quote it.

foreign/go/internal/vsr/envelope_test.go:164 - the unbound-partition test checks only the error; its metadata twin at :156 also pins that the rejection burns no id. that invariant is live now that partition ops draw ids.

core/server/src/dispatch.rs:1015 - stamping makes the checksum-mismatch deny path reachable for SendMessages for the first time. it's terminal, no retry, and nothing tests it.

and nothing anywhere sends n batches then retries a metadata op to prove the gapped sequence still dedups end to end. worth one case in core/integration/tests/sdk/.

Comment thread core/sdk/src/vsr.rs Outdated
Comment thread core/sdk/src/vsr.rs Outdated
Comment thread core/sdk/src/vsr.rs Outdated
Comment thread core/sdk/src/vsr.rs Outdated
Comment thread core/sdk/src/vsr.rs
Comment thread foreign/node/src/wire/vsr/index.ts
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Aug 27, 2026
@numinnex
numinnex force-pushed the fix_sdk_request_id_for_partition branch from a045ec0 to 92c9ce8 Compare August 27, 2026 13:53
@numinnex

Copy link
Copy Markdown
Contributor Author

/ready

@github-actions github-actions Bot added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review PR is waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants