Make prefill request timeout configurable #1377
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @varungup90, 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!
I've implemented changes to make the prefill request timeout configurable, moving away from a hardcoded value. This enhancement allows administrators to adjust the timeout period via an environment variable, providing greater control over request handling and improving system adaptability to varying network conditions or service latencies. Additionally, I've updated related gateway message timeouts to align with this new configurable setting.
Highlights
- Configurable Prefill Request Timeout: I've introduced a new environment variable, AIBRIX_PREFILL_REQUEST_TIMEOUT, which allows for dynamic configuration of the prefill request timeout. This replaces a previously hardcoded value, enhancing the system's flexibility.
- Configuration File Updates: I've updated the gateway configuration files (gateway-plugin.yaml, values.yaml, and vke.yaml) to include the new AIBRIX_PREFILL_REQUEST_TIMEOUT environment variable, setting its default value to "60".
- Increased Message Timeout: I've increased the messageTimeout in the gateway configurations from 5 seconds to 60 seconds across the relevant YAML files, aligning it with the new prefill request timeout.
- Go Code Adaptation for Timeout: The Go code has been modified to read the prefill request timeout from the AIBRIX_PREFILL_REQUEST_TIMEOUT environment variable. If the variable is not set, it defaults to 30 seconds, ensuring backward compatibility while providing configurability.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request makes the prefill request timeout configurable, which is a good improvement for flexibility. The implementation correctly introduces a new environment variable and uses it to set the HTTP client timeout. However, I've identified a potential issue in the configuration files where the Envoy messageTimeout is set to the same value as the new prefill request timeout. This could lead to race conditions and request failures. My review includes suggestions to adjust the messageTimeout in the YAML files to prevent this.
| @@ -200,4 +202,4 @@ spec: | |||
| body: Buffered | |||
| response: | |||
| body: Streamed | |||
| messageTimeout: 5s | |||
| messageTimeout: 60s | |||
There was a problem hiding this comment.
The messageTimeout for the Envoy extension policy is set to 60s, which is the same value as the AIBRIX_PREFILL_REQUEST_TIMEOUT (also set to 60 in this file). This can lead to race conditions where Envoy times out the request to the gateway plugin just as the internal prefill HTTP request is completing. The messageTimeout should be strictly greater than the prefill request timeout to allow for processing overhead. I suggest increasing this to 65s.
messageTimeout: 65s
| dependencies: | ||
| redis: | ||
| host: aibrix-redis-master | ||
| port: 6379 | ||
| messageTimeout: "5s" | ||
| messageTimeout: "60s" |
There was a problem hiding this comment.
The messageTimeout is set to "60s", which is the same value as AIBRIX_PREFILL_REQUEST_TIMEOUT. This can lead to race conditions where Envoy times out the request to the gateway plugin just as the internal prefill HTTP request is completing. The messageTimeout should be strictly greater than the prefill request timeout to allow for processing overhead. I suggest increasing this to 65s.
messageTimeout: "65s"
| dependencies: | ||
| redis: | ||
| host: aibrix-redis-master | ||
| port: 6379 | ||
| messageTimeout: "5s" | ||
| messageTimeout: "60s" |
There was a problem hiding this comment.
The messageTimeout is set to "60s", which is the same value as AIBRIX_PREFILL_REQUEST_TIMEOUT. This can lead to race conditions where Envoy times out the request to the gateway plugin just as the internal prefill HTTP request is completing. The messageTimeout should be strictly greater than the prefill request timeout to allow for processing overhead. I suggest increasing this to 65s.
messageTimeout: "65s"
Signed-off-by: Varun Gupta <varungup90@gmail.com>
Signed-off-by: Varun Gupta <varungup90@gmail.com>
Jeffwan
force-pushed
the
prefill-req-timeout
branch
from
5389de6 to
e1f3645
Compare
Pull Request Description
Related Issues
Resolves: #[Insert issue number(s)]
Important: Before submitting, please complete the description above and review the checklist below.
Contribution Guidelines (Expand for Details)
We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:
Pull Request Title Format
Your PR title should start with one of these prefixes to indicate the nature of the change:
[Bug]: Corrections to existing functionality[CI]: Changes to build process or CI pipeline[Docs]: Updates or additions to documentation[API]: Modifications to aibrix's API or interface[CLI]: Changes or additions to the Command Line Interface[Misc]: For changes not covered above (use sparingly)Note: For changes spanning multiple categories, use multiple prefixes in order of importance.
Submission Checklist
By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.