Conversation
ZhuLegend
left a comment
ZhuLegend
left a comment
There was a problem hiding this comment.
当前重构仅解决了长串if else的问题，并没有解决部分遗留问题，同时引入了新的问题。ServiceEnum应当提供的功能仅有一个：声明所有当前可用服务和服务相关的配置信息，同时尽可能避免错误。声明可用服务通过枚举类实现了，配置信息之前由长串的if else实现，现在由Handler实现，但是如何避免如漏写服务名或配置文件名或stub、试图为gRPC服务创建http的stub这类错误？
一个合适的方案是编写Enum的__new__和__init__方法，参考官方文档中为枚举添加实例属性的示例和何时使用 new() 与 init()。
通过__new__和__init__方法，我们可以在编写枚举时强制要求提供所需参数，如果同时使用dataclass作为需求参数，还可以为传入的参数提供名称以清晰的表明每个参数的含义。通过指定枚举value值，重写前后API和ServiceEnum行为不会发生任何变更，gRPCManager和HttpManager可能错误的创建stub可以通过将方法移动到ServiceEnum中来解决，这两个Manager可以简单的调用ServiceEnum的新方法来保证API的稳定性。使用上述方式重构可能是更好的，解决长串if else的方案。
| @property | ||
| def service_name(self) -> str: | ||
| if self == ServiceEnum.WechatManager: | ||
| return 'wechat_manager' | ||
| elif self == ServiceEnum.ApnsService: | ||
| return 'notification_center' | ||
| elif self == ServiceEnum.WechatService: | ||
| return 'notification_center' | ||
| elif self == ServiceEnum.NotificationService: | ||
| return 'notification_center' | ||
| elif self == ServiceEnum.ImportantInfoService: | ||
| return 'important_info' | ||
| elif self == ServiceEnum.MycquService: | ||
| return 'mycqu' | ||
| elif self == ServiceEnum.CardService: | ||
| return 'mycqu' | ||
| elif self == ServiceEnum.LibraryService: | ||
| return 'mycqu' | ||
| elif self == ServiceEnum.EduAdminCenter: | ||
| return 'edu_admin_center' | ||
| elif self == ServiceEnum.CourseScoreQuery: | ||
| return 'course_score_query' | ||
|
|
There was a problem hiding this comment.
service_name是公开的属性，在未对所有可能使用此方法的代码进行检查前最好不要直接删除该属性（保证API稳定性），而是考虑保留该属性的方案
| class gRPCManager(metaclass=Singleton): | ||
| def __init__(self, handler: ConfigHandler = _CONFIG_HANDLER): | ||
| # 初始化时创建每个ServiceHandler子类的实例 | ||
| self.service_instances = [cls() for cls in ServiceHandler.__subclasses__()] |
There was a problem hiding this comment.
不是所有服务都是使用gRPC进行编写的，考虑在这里通过is_http_service过滤不必要的实例；其次，反复调用子类构造方法会造成不必要的变量创建，ServiceEnum实际上的作用是将可用服务（如名称、所属配置文件）转化为枚举便于穷举和避免编写错误，起到了“配置”的作用，gRPC则通过“配置”创建服务的stub。既然是配置，那应该是唯一的，类似于单例，创建多个Handler实例指向唯一的“配置”似乎较为冗余。一种可行的方案是，为ServiceHandler创建一个私有类属性和一个静态方法，该静态方法接受一个filter（当前可以是简单的bool值，区分gRPC和http服务），在类属性为None时先获取所有实例并赋值给类属性，类属性有值时使用filter过滤不必要的实例回传以替代该方法，参考如下：
class ServiceHandler(ABC):
_subclass_instances = None
@staticmethod
def get_instances(is_grpc_service: bool) -> [ServiceHandler]
if ServiceHandler._subclass_instances is None:
ServiceHandler._subclass_instances = [cls() for cls in ServiceHandler.__subclasses__()]
return filter(lambda x: not is_http_service(x), ServiceHandler._subclass_instances)
注意，上述代码中的is_http_service方法由于当前代码使用Handler类而非ServiceEnum，需要考虑其他实现
| for instance in self.service_instances: | ||
| if instance.support(name=service): | ||
| return (self._service_host[instance.get_service_name + "_service_host"], | ||
| self._service_ports[instance.get_service_name + "_service_port"]) |
There was a problem hiding this comment.
未过滤Handler实例会在此处引入异常，可能会试图为http服务获取gRPC调用的配置（当然，之前的实现也存在这个问题）
| for instance in self.service_instances: | ||
| if instance.support(name=service): |
There was a problem hiding this comment.
同前，可能会为http服务创建gRPC的stub
There was a problem hiding this comment.
使用Handler解决了长串if else的问题，但是没有解决if else 可能存在遗漏的问题。例如，增加了一个新枚举但是忘记创建Handler的情况仍然存在。由于被分散到不同的文件，遗漏的可能性甚至增加了
2 participants
refactor(Service.py): 移除Service.py中获取stub和service_name的大量if else操作，改为handler适配器模式处理