-
Notifications
You must be signed in to change notification settings - Fork 590
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(udf): support client side load balancer for UDF #15200
Conversation
386ede3
to
d8cd658
Compare
d8cd658
to
02b9b47
Compare
May I ask whether there's any downside for always enabling the periodical service discovery? IIUC, if there's no loading balancing under the given domain name, it'll degrade to the normal case and everything should still work well. |
Yes, it will degrade to normal case.
If there's a L7 load balancer behind, the client-side load balancing is redundant, which issues an extra thread to refresh the IP list and has some extra cost on routing requests. |
I guess this is quite acceptable. Also IIUC, the interval for DNS lookup only impacts the responsiveness to the changes behind the L4 load balancer and does not hurt the functionality of loading balancing itself. Therefore, using a longer interval should also be fine. |
02b9b47
to
6ec6823
Compare
Update to always enabling client-side LB. The DNS refresh interval is set to be 5 seconds. |
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.
Rest LGTM
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.
LGTM!
src/expr/udf/src/external.rs
Outdated
pub fn connect_lazy(addr: &str) -> Result<Self> { | ||
let conn = tonic::transport::Endpoint::new(addr.to_string())? | ||
block_on(async { Self::connect_inner(addr, ResolutionStrategy::Lazy).await }) |
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.
By inspecting the source, it appears that .channel()
is actually synchronous. We may use .now_or_never().unwrap()
on the future here.
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.
Hmm will it be much higher cost to do block_on
+ await
here? I prefer not to rely on internal implementations of library.
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.
futures::executor::block_on
is fragile as well. If the future implementation leverages I/O capabilities of tokio
inside, it will also panic.
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.
Updated to .now_or_never().unwrap()
9366d56
to
b30d061
Compare
bb38a4c
to
e6a574c
Compare
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.
LGTM!
Co-authored-by: Kexiang Wang <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Support client side load balancer, which read IP list from DNS server and load balance requests to the IPs.
Background:
#15138
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.