-
Notifications
You must be signed in to change notification settings - Fork 24
Refactor Service routing into separate router classes #172
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
Changes from 4 commits
5c68d21
4a5ba51
11e76b4
9772764
9c7360b
35d9a12
d2cd105
eb32e55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
# Copyright (c) Meta Platforms, Inc. and affiliates. | ||
# All rights reserved. | ||
# | ||
# This source code is licensed under the BSD-style license found in the | ||
# LICENSE file in the root directory of this source tree. | ||
|
||
import logging | ||
from typing import Dict, List | ||
|
||
from .interface import Router | ||
from .replica import Replica | ||
|
||
logger = logging.getLogger(__name__) | ||
logger.setLevel(logging.DEBUG) | ||
|
||
|
||
class RoundRobinRouter(Router): | ||
"""Round-robin router for stateless requests.""" | ||
|
||
def __init__(self): | ||
self._next_idx = 0 | ||
|
||
def get_replica( | ||
self, | ||
replicas: List[Replica], | ||
sess_id: str | None = None, | ||
session_map: Dict[str, int] | None = None, | ||
) -> Replica: | ||
healthy_replicas = [r for r in replicas if r.healthy] | ||
if not healthy_replicas: | ||
allenwang28 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
raise RuntimeError("No healthy replicas available for load balancing") | ||
|
||
self._next_idx = (self._next_idx + 1) % len(healthy_replicas) | ||
replica = healthy_replicas[self._next_idx] | ||
|
||
return replica | ||
|
||
|
||
class LeastLoadedRouter(Router): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: BalancedRouter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion! See Allen's discussion on this in another thread.
I agree. So let's keep it as |
||
"""Always routes to the replica with the lowest current load.""" | ||
|
||
def get_replica( | ||
self, | ||
replicas: List["Replica"], | ||
DNXie marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
sess_id: str | None = None, | ||
session_map: Dict[str, int] | None = None, | ||
) -> "Replica": | ||
healthy_replicas = [r for r in replicas if r.healthy] | ||
if not healthy_replicas: | ||
raise RuntimeError("No healthy replicas available for session assignment") | ||
return min(healthy_replicas, key=lambda r: r.current_load) | ||
|
||
|
||
class SessionRouter(Router): | ||
"""Session-based routing: sticky sessions with a fallback router.""" | ||
|
||
def __init__(self, fallback_router: Router): | ||
self.fallback_router = fallback_router | ||
|
||
def get_replica( | ||
self, | ||
replicas: List["Replica"], | ||
DNXie marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
sess_id: str | None = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
i.e. don't assume There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is because I have a interface for routers: (also see
For RRRouter and LeastLoadedRouter, this could be None. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm ok, I'm not sure how I feel about that longer term but that's ok for now |
||
session_map: Dict[str, int] | None = None, | ||
) -> "Replica": | ||
if sess_id is None: | ||
raise ValueError("SessionRouter requires a session ID") | ||
|
||
if session_map is None: | ||
raise ValueError("Session map must be provided for SessionRouter") | ||
|
||
# Check if session already has a replica | ||
if sess_id in session_map: | ||
replica_idx = session_map[sess_id] | ||
# Find the replica with this index | ||
for r in replicas: | ||
if r.idx == replica_idx and r.healthy: | ||
return r | ||
# If the replica is no longer healthy, remove from session map and reassign | ||
del session_map[sess_id] | ||
|
||
# Use fallback router to assign a new replica | ||
replica = self.fallback_router.get_replica(replicas, sess_id, session_map) | ||
session_map[sess_id] = replica.idx | ||
logger.debug( | ||
"Assigning session %s to replica %d", | ||
sess_id, | ||
replica.idx, | ||
) | ||
return replica |
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.
nit - I think this Router can actually just be in router.py
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.
I think we cam keep it in interface.py since that's where all the interfaces are.