Skip to content

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

V0.2 #9

wants to merge 22 commits into from

Conversation

chnlkw
Copy link
Contributor

@chnlkw chnlkw commented Jun 29, 2025

Added

  • Added AsyncRead/AsyncWrite support for Tag and Stream (requires utils feature flag)
  • Added connect_addr_vec function to Worker

Changed

  • Updated to UCX 1.18 with latest API compatibility
  • Updated multiple dependency versions
  • Migrated to Rust 2021 edition

Fixed

  • Fixed various bugs and issues

@wangrunji0408 wangrunji0408 requested review from wangrunji0408 and Copilot and removed request for wangrunji0408 June 30, 2025 14:00
Copy link

@Copilot Copilot AI left a 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, and TagReadStream in endpoint/util.rs
  • Refactored request handling with a RequestParam builder and unified Status<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 of addr. Consider clarifying that this slice should come from worker.get_address() or rename to connect_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 be NoResource? Please confirm the correct spelling of this error variant.
            Error::NoReource => WouldBlock,

Copy link
Collaborator

@wangrunji0408 wangrunji0408 left a 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 😄

@chnlkw
Copy link
Contributor Author

chnlkw commented Jul 3, 2025

LGTM! Please resolve the issues mentioned by Copilot 😄

Thanks a lot. Code updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants