[router] Add builder pattern for RouterConfig with zero duplication#12030
[router] Add builder pattern for RouterConfig with zero duplication#12030
Conversation
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the configuration management for the router by implementing a fluent builder pattern for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed builder pattern for RouterConfig, significantly improving configuration ergonomics and maintainability across the codebase. By wrapping the RouterConfig struct, the builder avoids field duplication and provides a fluent, expressive API. The inclusion of maybe_* setters for optional fields and conditional boolean setters is a thoughtful touch that simplifies config construction. The refactoring has been applied consistently, cleaning up numerous manual struct instantiations in tests and application logic. I have one suggestion to further simplify the builder's usage in a test case. Overall, this is an excellent contribution to the project's code quality.
| let config = match mode { | ||
| RoutingMode::PrefillDecode { | ||
| prefill_urls, | ||
| decode_urls, | ||
| .. | ||
| } => RouterConfig::builder() | ||
| .prefill_decode_mode(prefill_urls, decode_urls) | ||
| .policy(policy) | ||
| .host("127.0.0.1") | ||
| .port(3001) | ||
| .max_payload_size(1024 * 1024) | ||
| .request_timeout_secs(60) | ||
| .worker_startup_timeout_secs(10) | ||
| .worker_startup_check_interval_secs(1) | ||
| .max_concurrent_requests(64) | ||
| .queue_timeout_secs(60) | ||
| .build_unchecked(), | ||
| _ => panic!("Expected PrefillDecode mode"), | ||
| }; |
There was a problem hiding this comment.
The match statement here is unnecessarily complex. Since you already have the mode and policy variables from the test_cases loop, you can directly use the .mode() and .policy() setters on the builder. This avoids deconstructing mode only to reconstruct it again, making the code simpler and more direct.
let config = RouterConfig::builder()
.mode(mode)
.policy(policy)
.host("127.0.0.1")
.port(3001)
.max_payload_size(1024 * 1024)
.request_timeout_secs(60)
.worker_startup_timeout_secs(10)
.worker_startup_check_interval_secs(1)
.max_concurrent_requests(64)
.queue_timeout_secs(60)
.build_unchecked();
Summary
Implements a fluent builder pattern for
RouterConfigthat eliminates code duplication and simplifies config construction across the codebase.Key Changes
1. Zero-Duplication Builder
Builder now wraps
RouterConfigdirectly instead of duplicating all 70+ fields:2. Setters
.maybe_*()methods for optional fields (eliminates if-let chains)3. Replaced 32 Manual Constructions
Replaced verbose manual config construction with builder pattern across:
Checklist