Open
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR bumps the crate to version 0.2.0, adds raw‐byte address connection support, and introduces full AsyncRead/AsyncWrite streams for tag- and stream-based communication under a new util feature.
- Added
Worker::connect_addr_vecfor connecting via raw address bytes - Introduced
WriteStream,ReadStream,TagWriteStream, andTagReadStreaminendpoint/util.rs - Refactored request handling with a
RequestParambuilder and unifiedStatus<T>abstraction
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ucx1-sys/ucx | Updated submodule commit for UCX 1.18 compatibility |
| src/ucp/worker.rs | Added connect_addr_vec and trace in Drop for WorkerAddress |
| src/ucp/endpoint/util.rs | Added AsyncRead/AsyncWrite stream wrappers with todo!() stubs |
| src/lib.rs | Impl Into<std::io::Error> for Error mappings |
Comments suppressed due to low confidence (3)
src/ucp/worker.rs:126
- [nitpick] The name
connect_addr_vecand its doc comment are vague about the expected format ofaddr. Consider clarifying that this slice should come fromworker.get_address()or rename toconnect_addr_bytesto better convey its purpose.
pub fn connect_addr_vec(self: &Rc<Self>, addr: &[u8]) -> Result<Endpoint, Error> {
src/ucp/worker.rs:126
- There are no tests covering the new
connect_addr_vecmethod. Consider adding a unit or integration test to verify connections via raw address bytes.
pub fn connect_addr_vec(self: &Rc<Self>, addr: &[u8]) -> Result<Endpoint, Error> {
src/lib.rs:173
- The variant
NoReourcelooks misspelled—should it beNoResource? Please confirm the correct spelling of this error variant.
Error::NoReource => WouldBlock,
wangrunji0408
approved these changes
Jun 30, 2025
Collaborator
wangrunji0408
left a comment
There was a problem hiding this comment.
LGTM! Please resolve the issues mentioned by Copilot 😄
Contributor
Author
Thanks a lot. Code updated |
5 tasks
ryanolson
added a commit
to ryanolson/async-ucx
that referenced
this pull request
Oct 25, 2025
Merges v0.2 branch introducing significant improvements: - Add AsyncRead/AsyncWrite support for Tag and Stream (utils feature) - Add Worker::connect_addr_vec for raw address connections - Update UCX to 1.18.1 for latest API compatibility - Introduce RequestParam builder pattern for request handling - Add stream wrapper types (WriteStream, ReadStream, etc.) - Migrate to Rust 2021 edition - Update dependencies and fix various bugs Signed-off-by: Ryan Olson <rolson@nvidia.com>
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.
Added
utilsfeature flag)connect_addr_vecfunction to WorkerChanged
Fixed