refactor(connector): replace hyper client implementation with reqwest #16146
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @BugenZhao and the rest of your teammates on |
BugenZhao
requested review from
xiangjinwu,
wenym1,
xxhZs and
xxchan
and removed request for
a team
| @@ -190,18 +190,9 @@ impl ClickHouseEngine { | |||
| } | |||
| } | |||
|
|
|||
| const POOL_IDLE_TIMEOUT: Duration = Duration::from_secs(5); | |||
There was a problem hiding this comment.
Is is ok to remove this POOL_IDLE_TIMEOUT setting? I guess the original purpose not to use ClickHouseClient::default() is to set this config. cc @xxhZs
There was a problem hiding this comment.
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 urls
There was a problem hiding this comment.
If so, we may not modify the code here? @BugenZhao
There was a problem hiding this comment.
the reason for not using the default here is to support https urls
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.
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.
Cherry-picked the fix into our fork of clickhouse.rs.
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
BugenZhao
force-pushed
the
bz/less-hyper-1
branch
from
f69fd95
to
86c3adf
Compare
BugenZhao
force-pushed
the
bz/less-hyper-2
branch
from
7a67c29
to
48e38c0
Compare
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
| .await | ||
| .map_err(|err| SinkError::DorisStarrocksConnect(err.into()))?; | ||
| // TODO: shall we let `reqwest` handle the redirect? |
There was a problem hiding this comment.
Sounds good, I wonder if the logic of the following special verdict is an obstacle cc @xuefengze
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
#15429 introduced another occurrence. 🤡🤡 |
Signed-off-by: Bugen Zhao <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
hyper::Clientis deprecated inhyper v1because it's considered to be higher-level.We'd better switch to
reqwestfor high-level HTTP client.Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.