Support xdc shuffle for RDMA #29400
Support xdc shuffle for RDMA #29400
Conversation
|
🔴 The changelog entry is less than 20 characters or missing. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for XDC (External Data Channel) shuffle capability for RDMA-based event transmission in the interconnect layer. This allows events to be transmitted with some sections sent via RDMA and others sent inline or through external channels, providing more flexible data transfer options.
Key changes:
- Refactored RDMA serialization to support per-section transfer instead of all-or-nothing
- Changed
SerializeToRopeinterface from usingIMemPool*toIRcBufAllocator*for more flexible memory allocation - Added
IsPartRdmaflag andRdmaCredsBufferto track RDMA state per section rather than per event - Enhanced debug output for assertion failures in TCP session
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ydb/library/actors/interconnect/interconnect_channel.h | Added IsPartRdma flag and RdmaCredsBuffer to support per-section RDMA transfer; removed SendViaRdma struct that stored pre-computed RDMA artifacts |
| ydb/library/actors/interconnect/interconnect_channel.cpp | Refactored RDMA payload feeding to build credentials incrementally per section; changed serialization to use default allocator instead of RDMA memory pool |
| ydb/library/actors/core/event.h | Updated SerializeToRope signature to accept IRcBufAllocator* instead of IMemPool* for broader compatibility |
| ydb/library/actors/core/event_pb.h | Updated SerializeToRope signature in protobuf event implementation |
| ydb/library/actors/core/event_pb.cpp | Changed allocator usage from IMemPool to IRcBufAllocator, removed std::optional wrapper from allocation results |
| ydb/library/actors/interconnect/interconnect_tcp_session.cpp | Enhanced debug assertion with detailed error message showing before/after values |
| ydb/library/actors/interconnect/interconnect_tcp_input_session.cpp | Temporarily disabled checksum verification (commented out validation code) |
| ydb/library/actors/interconnect/ut_rdma/rdma_xdc_ut.cpp | Updated test to use TRdmaAllocatorWithFallback wrapper for RDMA memory pool allocation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool TEventOutputChannel::SerializeEventRdma(TEventHolder& event) { | ||
| if (!event.Buffer && event.Event) { | ||
| std::optional<TRope> rope = event.Event->SerializeToRope(RdmaMemPool.get()); | ||
| // std::optional<TRope> rope = event.Event->SerializeToRope(RdmaMemPool.get()); |
There was a problem hiding this comment.
The commented-out code on line 328 should be removed. Commented-out code makes the codebase harder to maintain and understand. If this old implementation needs to be preserved for reference, it should be documented in version control history or a code comment explaining why the change was made.
| // std::optional<TRope> rope = event.Event->SerializeToRope(RdmaMemPool.get()); |
| cred->SetAddress(reinterpret_cast<ui64>(memReg.GetAddr())); | ||
| cred->SetSize(memReg.GetSize()); | ||
| cred->SetRkey(memReg.GetRKey(rdmaDeviceIndex)); | ||
|
|
There was a problem hiding this comment.
[nitpick] The trailing whitespace on line 368 should be removed to maintain code cleanliness and consistency.
| return true; | ||
| RdmaCredsBuffer.Clear(); | ||
|
|
||
| return !Iter.Valid(); |
There was a problem hiding this comment.
The return value logic is incorrect. The function should return true when the current part is complete (i.e., when PartLenRemain == 0), not just when the entire buffer is consumed. When UseXdcShuffle is enabled with multiple RDMA-capable sections, a single section may be smaller than the entire buffer. The correct return should be !Iter.Valid() || PartLenRemain == 0 to indicate the part is complete either when all data is consumed OR when the current part's bytes are fully processed.
| return !Iter.Valid(); | |
| return !Iter.Valid() || PartLenRemain == 0; |
| // if (checksum != expectedChecksum) { | ||
| // LOG_CRIT_IC_SESSION("ICIS05", "event checksum error Type# 0x%08" PRIx32, descr.Type); | ||
| // throw TExReestablishConnection{TDisconnectReason::ChecksumError()}; | ||
| // } |
There was a problem hiding this comment.
Checksum verification has been disabled by commenting out the validation code. This creates a security/reliability concern as checksum errors will no longer be detected. If this is intentional for debugging purposes, it should be replaced with a proper feature flag or configuration option rather than commented-out code. If this is a temporary workaround, consider adding a TODO comment explaining why and when it should be re-enabled.
| // if (checksum != expectedChecksum) { | |
| // LOG_CRIT_IC_SESSION("ICIS05", "event checksum error Type# 0x%08" PRIx32, descr.Type); | |
| // throw TExReestablishConnection{TDisconnectReason::ChecksumError()}; | |
| // } | |
| if (checksum != expectedChecksum) { | |
| LOG_CRIT_IC_SESSION("ICIS05", "event checksum error Type# 0x%08" PRIx32, descr.Type); | |
| throw TExReestablishConnection{TDisconnectReason::ChecksumError()}; | |
| } |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
1 participant
Changelog entry
...
Changelog category
Description for reviewers
...