-
Notifications
You must be signed in to change notification settings - Fork 18
V0.2 #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
V0.2 #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Please resolve the issues mentioned by Copilot 😄
Thanks a lot. Code updated |
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>
Added
utilsfeature flag)connect_addr_vecfunction to WorkerChanged
Fixed