extend load balancer context interface to provide primary host#17242
extend load balancer context interface to provide primary host#17242wbpcode wants to merge 3 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for the contribution. This is much too large of a change to regression prone components to not have more information attached to it. Please provide more information about the overall design, optimally in the form of a short google doc. Thank you!
/wait
|
Thanks. @mattklein123 https://docs.google.com/document/d/1IU4b76AgOXijNa4sew1gfBfSiOMbZNiEt5Dhis8QpYg/edit?usp=sharing In fact, this document already exists in #16698. This time I just added some specific implementation designs. I'm not a native speaker. So if the documentation is unclear, please feel free to point it. |
Signed-off-by: wbpcode <wbphub@live.com>
|
/retest |
|
Retrying Azure Pipelines: |
Thanks. If there is a suitable plan to achieve this demand, I will be happy to accept it. I think we can talk in detail on slack. |
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for working on this. At a high level I agree with the intention, but I would like to simplify and unify the implementation.
Let's start by doing a PR to only add the all_host_map into the central priority set and get it propagating. After that we can discuss the various LB changes in other PRs.
/wait
| * health status of the host matches the expectation, the load balancer can bypass the load | ||
| * balancing algorithm and return the corresponding host directly. | ||
| */ | ||
| virtual absl::optional<ExpectedHost> primaryHostShouldSelected() const PURE; |
There was a problem hiding this comment.
I would call this function overrideHostToSelect(). I think that will be more clear. "primary" host I don't think is very helpful. Can we update this?
| * string. If the PrioritySet does not provide the fast searching of host, this method returns | ||
| * nullptr. |
There was a problem hiding this comment.
I don't think it makes sense to only make this work for EDS. We should make this work for all cluster types if we are going to do this. See my other comment below.
| std::vector<LocalityWeightsMap> locality_weights_map_; | ||
| HostMap all_hosts_; | ||
|
|
||
| HostMapSharedPtr all_host_map_; |
There was a problem hiding this comment.
Instead of having this be in the EDS cluster only, I would recommend moving this into the cluster base class, or directly in the priority set. This way it will be used and populated by all cluster types (per my other comment). Additionally, if we do this, I think we can remove all of the special case code you added in cluster manager impl. It's just more data that should be updated and posted on updates. I don't fully see the value of all that new code.
There was a problem hiding this comment.
@mattklein123
I added the postThreadLocalHostMapUpdate method to the clusterManager to ensure that all_host_map_ in the EDS Cluster can be synchronized to each worker thread after the update. When batchUpdate is used, postThreadLocalClusterUpdate cannot do this.
If you maintain all_host_map_ directly in PrioritySet, it is indeed possible to remove these codes. But it may bring some frequent HostMap copy problems (I'm not entirely sure).
But I agree that we can implement a general all_host_map first, and then continue. So we can continue to discuss this issue in the new PR.
There was a problem hiding this comment.
IMO it won't be too costly to maintain a map in the priority set and then update it when we update everything else. Let's try that first and optimize later if needed?
There was a problem hiding this comment.
Okay, I will prepare a new PR as soon as possible this week. 👌
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Commit Message: extend load balancer context interface to provide primary host
Additional Description:
Extend load balancer context interface to provide primary host and allow the load balancer to skip the load balancing calculation and directly select the eligible primary host.
This is one of the most critical tasks related to #16698. After completing the work, we can complete #16698 by extending the specific implementation of LocalBalancerContext.
Risk Level: Medium.
Testing: Added.
Docs Changes: N/A.
Release Notes: N/A.