Skip to content
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

feat: Bidirectional streaming for source transformer #91

Merged
merged 10 commits into from
Oct 1, 2024

Conversation

BulkBeing
Copy link
Collaborator

@BulkBeing BulkBeing commented Sep 25, 2024

src/sourcetransform.rs Outdated Show resolved Hide resolved

const DEFAULT_MAX_MESSAGE_SIZE: usize = 64 * 1024 * 1024;
const DEFAULT_SOCK_ADDR: &str = "/var/run/numaflow/sourcetransform.sock";
const DEFAULT_SERVER_INFO_FILE: &str = "/var/run/numaflow/sourcetransformer-server-info";
const DEFAULT_CHANNEL_SIZE: usize = 1000;

const DROP: &str = "U+005C__DROP__";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub mod proto rename proto to sourcetransformer_pb ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is needed since the it's absolute name (crate::sourcetransform::proto) already makes the namespacing clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yhl25 had some opinions on this.

src/sourcetransform.rs Outdated Show resolved Hide resolved
return Err(Status::invalid_argument("Handshake not present"));
}

let handle: JoinHandle<Result<(), Error>> = tokio::spawn({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please document your though

src/sourcetransform.rs Outdated Show resolved Hide resolved
src/sourcetransform.rs Outdated Show resolved Hide resolved
src/sourcetransform.rs Outdated Show resolved Hide resolved
src/sourcetransform.rs Outdated Show resolved Hide resolved
src/sourcetransform.rs Outdated Show resolved Hide resolved
src/sourcetransform.rs Outdated Show resolved Hide resolved
src/sourcetransform.rs Outdated Show resolved Hide resolved
src/sourcetransform.rs Outdated Show resolved Hide resolved
@yhl25 yhl25 merged commit 30d8ce1 into numaproj:main Oct 1, 2024
2 checks passed
@BulkBeing BulkBeing deleted the source-transformer-streaming branch October 1, 2024 03:36
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.

3 participants