-
Notifications
You must be signed in to change notification settings - Fork 596
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
refactor(connector): replace hyper client implementation with reqwest #16146
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
8cf9271
refactor(metrics): replace hyper implementation with axum
BugenZhao 86c3adf
remove hyper dependency on dashboard proxy
BugenZhao 5b2825b
doris
BugenZhao 7b59b91
doris starrocks connector
BugenZhao 48e38c0
use default client for clickhouse client
BugenZhao 9dc3caa
bump clickhouse fork rev
BugenZhao f43dcb1
fix redirection
BugenZhao 87d0797
Merge remote-tracking branch 'origin/main' into bz/less-hyper-2
BugenZhao 2f1f8a3
fix snowflake
BugenZhao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,16 +15,15 @@ | |
use core::mem; | ||
use core::time::Duration; | ||
use std::collections::HashMap; | ||
use std::convert::Infallible; | ||
|
||
use anyhow::Context; | ||
use base64::engine::general_purpose; | ||
use base64::Engine; | ||
use bytes::{BufMut, Bytes, BytesMut}; | ||
use http::request::Builder; | ||
use hyper::body::{Body, Sender}; | ||
use hyper::client::HttpConnector; | ||
use hyper::{body, Client, Request, StatusCode}; | ||
use hyper_tls::HttpsConnector; | ||
use futures::StreamExt; | ||
use reqwest::{redirect, Body, Client, RequestBuilder, StatusCode}; | ||
use tokio::sync::mpsc::UnboundedSender; | ||
use tokio::task::JoinHandle; | ||
use url::Url; | ||
|
||
|
@@ -187,33 +186,27 @@ impl InserterInnerBuilder { | |
}) | ||
} | ||
|
||
// TODO: use hyper 1 or reqwest 0.12.2 | ||
fn build_request_and_client( | ||
&self, | ||
uri: String, | ||
) -> (Builder, Client<HttpsConnector<HttpConnector>>) { | ||
let mut builder = Request::put(uri); | ||
for (k, v) in &self.header { | ||
builder = builder.header(k, v); | ||
} | ||
|
||
let connector = HttpsConnector::new(); | ||
fn build_request(&self, uri: String) -> RequestBuilder { | ||
let client = Client::builder() | ||
.pool_idle_timeout(POOL_IDLE_TIMEOUT) | ||
.build(connector); | ||
.redirect(redirect::Policy::none()) // we handle redirect by ourselves | ||
.build() | ||
.unwrap(); | ||
|
||
(builder, client) | ||
let mut builder = client.put(uri); | ||
for (k, v) in &self.header { | ||
builder = builder.header(k, v); | ||
} | ||
builder | ||
} | ||
|
||
pub async fn build(&self) -> Result<InserterInner> { | ||
let (builder, client) = self.build_request_and_client(self.url.clone()); | ||
let request_get_url = builder | ||
.body(Body::empty()) | ||
.map_err(|err| SinkError::DorisStarrocksConnect(err.into()))?; | ||
let resp = client | ||
.request(request_get_url) | ||
let builder = self.build_request(self.url.clone()); | ||
let resp = builder | ||
.send() | ||
.await | ||
.map_err(|err| SinkError::DorisStarrocksConnect(err.into()))?; | ||
// TODO: shall we let `reqwest` handle the redirect? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, I wonder if the logic of the following special verdict is an obstacle cc @xuefengze |
||
let mut be_url = if resp.status() == StatusCode::TEMPORARY_REDIRECT { | ||
resp.headers() | ||
.get("location") | ||
|
@@ -249,23 +242,25 @@ impl InserterInnerBuilder { | |
} | ||
} | ||
|
||
let (builder, client) = self.build_request_and_client(be_url); | ||
let (sender, body) = Body::channel(); | ||
let request = builder | ||
.body(body) | ||
.map_err(|err| SinkError::DorisStarrocksConnect(err.into()))?; | ||
let future = client.request(request); | ||
let (sender, receiver) = tokio::sync::mpsc::unbounded_channel(); | ||
let body = Body::wrap_stream( | ||
tokio_stream::wrappers::UnboundedReceiverStream::new(receiver).map(Ok::<_, Infallible>), | ||
); | ||
let builder = self.build_request(be_url).body(body); | ||
|
||
let handle: JoinHandle<Result<Vec<u8>>> = tokio::spawn(async move { | ||
let response = future | ||
let response = builder | ||
.send() | ||
.await | ||
.map_err(|err| SinkError::DorisStarrocksConnect(err.into()))?; | ||
let status = response.status(); | ||
let raw = body::to_bytes(response.into_body()) | ||
let raw = response | ||
.bytes() | ||
.await | ||
.map_err(|err| SinkError::DorisStarrocksConnect(err.into()))? | ||
.to_vec(); | ||
if status == StatusCode::OK && !raw.is_empty() { | ||
.into(); | ||
|
||
if status == StatusCode::OK { | ||
Ok(raw) | ||
} else { | ||
Err(SinkError::DorisStarrocksConnect(anyhow::anyhow!( | ||
|
@@ -280,6 +275,8 @@ impl InserterInnerBuilder { | |
} | ||
} | ||
|
||
type Sender = UnboundedSender<Bytes>; | ||
|
||
pub struct InserterInner { | ||
sender: Option<Sender>, | ||
join_handle: Option<JoinHandle<Result<Vec<u8>>>>, | ||
|
@@ -301,37 +298,18 @@ impl InserterInner { | |
|
||
let chunk = mem::replace(&mut self.buffer, BytesMut::with_capacity(BUFFER_SIZE)); | ||
|
||
let is_timed_out = match tokio::time::timeout( | ||
SEND_CHUNK_TIMEOUT, | ||
self.sender.as_mut().unwrap().send_data(chunk.into()), | ||
) | ||
.await | ||
{ | ||
Ok(Ok(_)) => return Ok(()), | ||
Ok(Err(_)) => false, | ||
Err(_) => true, | ||
}; | ||
self.abort()?; | ||
if let Err(_e) = self.sender.as_mut().unwrap().send(chunk.freeze()) { | ||
self.sender.take(); | ||
self.wait_handle().await?; | ||
|
||
let res = self.wait_handle().await; | ||
|
||
if is_timed_out { | ||
Err(SinkError::DorisStarrocksConnect(anyhow::anyhow!("timeout"))) | ||
} else { | ||
res?; | ||
Err(SinkError::DorisStarrocksConnect(anyhow::anyhow!( | ||
"channel closed" | ||
))) | ||
} else { | ||
Ok(()) | ||
} | ||
} | ||
|
||
fn abort(&mut self) -> Result<()> { | ||
if let Some(sender) = self.sender.take() { | ||
sender.abort(); | ||
} | ||
Ok(()) | ||
} | ||
|
||
pub async fn write(&mut self, data: Bytes) -> Result<()> { | ||
self.buffer.put_slice(&data); | ||
if self.buffer.len() >= MIN_CHUNK_SIZE { | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is is ok to remove this
POOL_IDLE_TIMEOUT
setting? I guess the original purpose not to useClickHouseClient::default()
is to set this config. cc @xxhZsThere 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.
The corresponding pr is this one, https://github.com/risingwavelabs/risingwave/pull/12041/files
If we do not set, the default value of
POOL_IDLE_TIMEOUT
is 2s in clickhouse.rs, the reason for not using the default here is to support https urlsThere 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.
If so, we may not modify the code here? @BugenZhao
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.
I didn't find the clue for this. The reason why #12041 works is explained in ClickHouse/clickhouse-rs#58 (comment). There seems nothing to do with the
POOL_IDLE_TIMEOUT
.BTW, the upstream has submitted a fix (risingwavelabs/clickhouse.rs@3fc12f6) and there's no more need for this workaround. Should we sync the changes instead?
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.
May need to run a test to connect to clickhouse with https. If it passes, we can then sync the change.
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.
Cherry-picked the fix into our fork of
clickhouse.rs
.