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

[Merged by Bors] - Initial stuff for Fluvio::connect_with_connector #1120

Closed

Conversation

simlay
Copy link
Contributor

@simlay simlay commented May 25, 2021

I also added in a wasm test. I've got mixed feelings about where it is now.

@simlay simlay requested a review from sehz May 25, 2021 04:08
@sehz
Copy link
Contributor

sehz commented May 25, 2021

rustfmt is failing

Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

LGTM

@sehz
Copy link
Contributor

sehz commented May 25, 2021

local cluster test is failing

@simlay
Copy link
Contributor Author

simlay commented May 25, 2021

local cluster test is failing

"Expected browser binary location, but unable to find binary in default location, no 'moz:firefoxOptions.binary' capability provided, and no binary flag set on the command line"

@nacardin is firefox or chrome installed on the custom runner?

@sehz
Copy link
Contributor

sehz commented May 25, 2021

Suggest separating integration test as 2nd PR so code changes can go thru

Comment on lines +70 to +73
pub async fn connect_with_connector(
connector: DomainConnector,
config: &FluvioConfig,
) -> Result<Self, FluvioError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding this? And more importantly, what is the usage pattern here? Are we expecting typical users to leverage this or is it more of an implementation detail for the WASM library that users will not directly use?

Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

This allows client to use custom networking. For example: unix socket, websocket, QUIC, etc.

let producer = client.topic_producer(topic.clone()).await;
assert!(producer.is_ok());
let producer = producer.unwrap();
let send = producer.send("foo", "bar").await;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a work around because #986 is still an issue.

@sehz
Copy link
Contributor

sehz commented May 25, 2021

will merge this after release

@sehz
Copy link
Contributor

sehz commented May 25, 2021

bors r+

bors bot pushed a commit that referenced this pull request May 25, 2021
I also added in a wasm test. I've got mixed feelings about where it is now.
@bors bors bot changed the title Initial stuff for Fluvio::connect_with_connector [Merged by Bors] - Initial stuff for Fluvio::connect_with_connector May 25, 2021
@bors bors bot closed this May 25, 2021
@sehz
Copy link
Contributor

sehz commented May 26, 2021

Move changelog for this to 0.8.4

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