refactor(csharp): update integer properties to unsigned types for consistency - #4027
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4027 +/- ##
============================================
+ Coverage 85.05% 85.10% +0.04%
Complexity 1402 1402
============================================
Files 1237 1236 -1
Lines 181722 181699 -23
Branches 148015 148003 -12
============================================
+ Hits 154567 154629 +62
+ Misses 23098 23047 -51
+ Partials 4057 4023 -34
🚀 New features to boost your workflow:
|
99bbcdc to
bb05981
Compare
hubcio
left a comment
There was a problem hiding this comment.
outside the diff:
TcpContracts.cs:282-stream.Topics != nullhas the same empty-dictionary bug you fixed forStreams: an emptyTopicswriteshas_topics = 1with no entries and the server rejects the request. sameis { Count: > 0 }here and at line 347.TcpContracts.cs:151-UpdatePermissions(userId, null)sizes the buffer without the has-permissions byte, then writes it at line 167. index out of range on the documented defaultpermissions = null.BinaryFactory.cs:155-CreatePartitionPayloadhas no callers and throws on first use (16-byte buffer, 8-byte writes at offsets 12 and 16). delete it.
spetz
left a comment
There was a problem hiding this comment.
Looks good, here are 2 extra things to improve outisde the scope of this PR, so feel free to either update this PR or we can add these to the next one.
foreign/csharp/Iggy_SDK/Vsr/ConsumerGroupClientState.cs:46 - an empty cached assignment is treated as missing, causing repeated synchronization and a possible hot loop when PollingIntervalMs is 0. Consider preserving empty assignments and returning the 0xFFFF_FFFE no-assignment sentinel with backoff.
foreign/csharp/Iggy_SDK/Mappers/BinaryMapper.cs:803 - the options-length prefix is read without confirming four bytes remain, so truncated input throws ArgumentOutOfRangeException instead of MalformedResponseException. Consider reusing ReadLength.
3f48365 to
3ad1049
Compare
4f22049 to
abef087
Compare
|
2 extra comments from @spetz was also done in this PR |
There was a problem hiding this comment.
went through all 21 comments from the last round against the code - every one is genuinely fixed, and the unit suite is green locally (1028 tests across net8.0 and net10.0, 0 build warnings). the MapHeaders rewrite onto shared ReadHeaderKind / ReadHeaderField and the ArrayBufferWriter permissions pass came out cleaner than what I asked for.
two things live outside the diff so I could not anchor them:
Iggy_SDK_Tests/VsrTests/SyncConsumerGroupTests.cs - nothing covers the sentinel this fix is built on. neither the PollGroupMessagesRentedAsync return nor the consumer backoff has a test, while go covers both in tcp_group_polling_test.go:262 and :279. the assert that actually guards the HasAssignment change is that a second poll issues no new SYNC_CONSUMER_GROUP.
Iggy_SDK/IggyClient/IIggyConsumer.cs:45 and :57 - PollMessagesAsync and PollMessagesRentedAsync are public and do not say PartitionId can come back as NO_ASSIGNED_PARTITION with an empty batch. go states it in the interface contract (contracts/client.go:128). anyone using the raw poll instead of IggyConsumer would key offsets on partition 4294967294.
No description provided.