rest.li
rest.li copied to clipboard
Add getClient async call in warmup load balancer
Background Recently voyager-api found another blocking call that caused a host being shutdown manually after all threads are blocked. After taking a look into the log, we found that there is another blocking call when calling getClient. In LoadBalancer.java, there are 2 methods in this interface: 1) getClient, 2) getLoadBalancedServiceProperties, both methods have sync and async implementations. Last time we fixed getLoadBalancedServiceProperties by letting the last client going through as async path, however, looks like getClient method is another source that leads to blocked threads.
How to fix After looking into the code, we believe warmUpLoadBalancer is the only load balancer that is making the sync call to get client, the upstream class makes call using the async way, however, because warmUpLoadBalancer and LoadBalancerWithFacilitiesDelegator do not have the async implementation, it defaults to LoadBalancer.java, which is faking the async implementation by waiting for the response. Therefore, as long as we add the async implementation of getClient, the problem should be solved.
Please note, this change is not guarded by any config, since the change is fairly straight forward.
Update
- I removed the default implementation of the async path since the default implementation is faking it! This is backward compatible change, it will break a small amount of mps - container(unit test only), restli-test-fwk and autortf. Because it does not impact large number of upstreams, I would rather make this backward incompatible change and force everyone to have the truly async implementation.
- I didn't mark the synchronous path @deprecated because there are so many dependencies on the sync path, it will be a ton of code cleanups.