Refactor Service routing into separate router classes #172
Conversation
meta-cla
bot
added
the
CLA Signed
DNXie
changed the title
DNXie
changed the title
| def get_replica( | ||
| self, | ||
| replicas: List["Replica"], | ||
| sess_id: str | None = None, |
There was a problem hiding this comment.
sess_id: str,
i.e. don't assume None is passable for this or session_map. I would also get rid of the checks below
There was a problem hiding this comment.
This is because I have a interface for routers: (also see interface.py)
class Router(ABC):
"""Abstract base class for routing logic."""
@abstractmethod
def get_replica(
self,
healthy_replicas: List[Replica],
sess_id: str | None = None,
session_map: Dict[str, int] | None = None,
) -> Replica:
"""Select a replica from the list based on routing logic."""
pass
For RRRouter and LeastLoadedRouter, this could be None.
There was a problem hiding this comment.
hmm ok, I'm not sure how I feel about that longer term but that's ok for now
|
|
||
| @pytest.mark.timeout(10) | ||
| @pytest.mark.asyncio | ||
| async def test_round_robin_router_distribution(): |
There was a problem hiding this comment.
for these router integration tests, are these not already covered in the above tests?
There was a problem hiding this comment.
Most functionalities are covered. But the above tests are only testing router itself. The integration tests are testing routing behaviors through a service. If you have concerns about the CI overhead, we could get ride of these except test_round_robin_router_distribution, which is the only test for roundrobin.
There was a problem hiding this comment.
I removed one integration tests and 2 other unit tests due to overlapped coverage, so now just one unit test and two integration tests:
test_session_router_with_round_robin_fallback: test different fallback router for the SessionRouter. Also tests the correctness ofLeastLoadedRouter.test_round_robin_router_distribution(integration): the only test case for RR logictest_session_router_assigns_and_updates_session_map_in_service(integration): is to test whether the session_map modified in Router can get updated properly in Service class.
So I think we should keep both.
| return replica | ||
|
|
||
|
|
||
| class LeastLoadedRouter(Router): |
There was a problem hiding this comment.
I think LeastConnectedRouter would be the most canonically accurate
There was a problem hiding this comment.
Thanks for the suggestion!
See Allen's discussion on this in another thread.
I think
LeastConnectedRouterwould be the most canonically accurate
I agree. So let's keep it as LeastConnectedRouter for now.
| ) | ||
|
|
||
|
|
||
| class Router(ABC): |
There was a problem hiding this comment.
nit - I think this Router can actually just be in router.py
There was a problem hiding this comment.
I think we cam keep it in interface.py since that's where all the interfaces are.
| return replica | ||
|
|
||
|
|
||
| class LeastLoadedRouter(Router): |
There was a problem hiding this comment.
I think LeastConnectedRouter would be the most canonically accurate
| def get_replica( | ||
| self, | ||
| replicas: List["Replica"], | ||
| sess_id: str | None = None, |
There was a problem hiding this comment.
hmm ok, I'm not sure how I feel about that longer term but that's ok for now
|
|
||
| @pytest.mark.timeout(10) | ||
| @pytest.mark.asyncio | ||
| async def test_round_robin_router_distribution(): |
3 participants
This PR refactors the Service routing logic into separate router classes.
Changes:
RoundRobinRouter,LeastLoadedRouter, andSessionRouter.Routerinterface.Serviceclass accordingly.SessionRouterandLeastLoadedRouterbehavior and fallback routing.Test: