-
Notifications
You must be signed in to change notification settings - Fork 17
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_vec
for connecting via raw address bytes - Introduced
WriteStream
,ReadStream
,TagWriteStream
, andTagReadStream
inendpoint/util.rs
- Refactored request handling with a
RequestParam
builder 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_vec
and its doc comment are vague about the expected format ofaddr
. Consider clarifying that this slice should come fromworker.get_address()
or rename toconnect_addr_bytes
to 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_vec
method. 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
NoReource
looks 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 |
Added
utils
feature flag)connect_addr_vec
function to WorkerChanged
Fixed