feat(servers): make http timeout and body limit optional #4217
Conversation
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
WalkthroughThe recent updates enhance the HTTP server configuration options, allowing more flexibility by enabling the timeout and body limit settings to be disabled by setting their values to 0. This change affects configuration files and the HTTP server's implementation, providing improved configurability and control. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
fengjiachun
changed the title
fengjiachun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- config/config.md (2 hunks)
- config/frontend.example.toml (1 hunks)
- config/standalone.example.toml (1 hunks)
- src/servers/src/http.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- config/standalone.example.toml
Additional context used
LanguageTool
config/config.md
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | |mysql| -- | -- | MySQL server o...
[uncategorized] ~40-~40: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |postgres| -- | -- | PostgresSQL ...
[uncategorized] ~49-~49: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |opentsdb| -- | -- | OpenTSDB pro...
[grammar] ~93-~93: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...
[grammar] ~94-~94: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g |None| The credential path of the google cloud storage.
**It's only used whe...
[uncategorized] ~108-~108: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: .... If not set, it's default to 1/8 of OS memory with a max limitation of 1GB. | | `regi...
[style] ~134-~134: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~143-~143: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...
[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | |mysql| -- | -- | MySQL server o...
[uncategorized] ~187-~187: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |postgres| -- | -- | PostgresSQL ...
[uncategorized] ~196-~196: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |opentsdb| -- | -- | OpenTSDB pro...
[style] ~220-~220: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~229-~229: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
[style] ~282-~282: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~291-~291: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...
[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | |runtime| -- | -- | The runtime ...
[grammar] ~373-~373: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...
[grammar] ~374-~374: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g |None| The credential path of the google cloud storage.
**It's only used whe...
[uncategorized] ~388-~388: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: .... If not set, it's default to 1/8 of OS memory with a max limitation of 1GB. | | `regi...
[style] ~414-~414: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~423-~423: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
Markdownlint
config/config.md
5-5: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
6-6: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
7-7: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
151-151: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
237-237: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
299-299: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
37-37: null (MD034, no-bare-urls)
Bare URL used
184-184: null (MD034, no-bare-urls)
Bare URL used
246-246: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (3)
config/frontend.example.toml (2)
29-29: Clarification on timeout setting.The comment "Set to 0 to disable timeout." is clear and directly informs users of the new functionality. This is a significant improvement for flexibility in configuration.
33-33: Clarification on body limit setting.The addition of "Set to 0 to disable limit." enhances user control over HTTP request body size, aligning with the changes described in the summary. This update is valuable for environments where large or unrestricted body sizes are necessary.
src/servers/src/http.rs (1)
673-688: Review of dynamic layer configuration in HTTP server build method.The conditional creation of
timeout_layerandbody_limit_layerbased on the server options is a robust design choice. It allows for greater flexibility and customization of server behavior based on configuration values. The use ofServiceBuilderto conditionally apply these layers is a clean and effective implementation. However, ensure that the logging statements provide enough information for debugging purposes.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- config/config.md (2 hunks)
- config/frontend.example.toml (1 hunks)
- config/standalone.example.toml (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- config/frontend.example.toml
- config/standalone.example.toml
Additional context used
LanguageTool
config/config.md
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | |mysql| -- | -- | MySQL server o...
[uncategorized] ~40-~40: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |postgres| -- | -- | PostgresSQL ...
[uncategorized] ~49-~49: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |opentsdb| -- | -- | OpenTSDB pro...
[grammar] ~93-~93: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...
[grammar] ~94-~94: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g |None| The credential path of the google cloud storage.
**It's only used whe...
[uncategorized] ~108-~108: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: .... If not set, it's default to 1/8 of OS memory with a max limitation of 1GB. | | `regi...
[style] ~134-~134: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~143-~143: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...
[uncategorized] ~178-~178: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | |mysql| -- | -- | MySQL server o...
[uncategorized] ~187-~187: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |postgres| -- | -- | PostgresSQL ...
[uncategorized] ~196-~196: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload | |opentsdb| -- | -- | OpenTSDB pro...
[style] ~220-~220: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~229-~229: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
[style] ~282-~282: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~291-~291: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...for Certificate and key file change and auto reload.
For now, gRPC tls config does not ...
[uncategorized] ~325-~325: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...r now, gRPC tls config does not support auto reload. | |runtime| -- | -- | The runtime ...
[grammar] ~373-~373: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...e| String |None` | The scope of the google cloud storage.
**It's only used whe...
[grammar] ~374-~374: “Google” is a proper noun and needs to be capitalized. (A_GOOGLE)
Context: ...g |None| The credential path of the google cloud storage.
**It's only used whe...
[uncategorized] ~388-~388: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: .... If not set, it's default to 1/8 of OS memory with a max limitation of 1GB. | | `regi...
[style] ~414-~414: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...vel| String |None| The log level. Can beinfo/debug/warn/error. | |...
[grammar] ~423-~423: Consider using either the past participle “recommended” or the present participle “recommending” here. (BEEN_PART_AGREEMENT)
Context: ...Forstandalonemode,self_importis recommend to collect metrics generated by itself ...
Markdownlint
config/config.md
5-5: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
6-6: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
7-7: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
151-151: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
237-237: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
299-299: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
37-37: null (MD034, no-bare-urls)
Bare URL used
184-184: null (MD034, no-bare-urls)
Bare URL used
246-246: null (MD034, no-bare-urls)
Bare URL used
Additional comments not posted (4)
config/config.md (4)
22-22: Updated HTTP timeout configuration documentationThe documentation now clearly specifies that setting the
http.timeoutto0disables the timeout. This is a crucial update for users who need flexible configuration, especially in environments where timeouts may not be desired.
23-23: Enhanced HTTP body limit configurationThe update to
http.body_limitincludes detailed support for various units and the option to disable the limit by setting it to0. This enhancement not only improves usability but also aligns with the need for more dynamic server configurations.
169-169: Consistency in HTTP timeout configuration across modesThe addition of disabling the timeout feature in the
Distributed Modesection ensures consistency across different operational modes, which is beneficial for maintaining uniform behavior across deployments.
170-170: Unified approach to HTTP body limit settingsSimilar to the standalone mode, the distributed mode now also supports the detailed specification of units for
http.body_limitand the ability to disable it. This uniformity across configurations is excellent for user experience and system administration.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4217 +/- ##
==========================================
- Coverage 84.85% 84.56% -0.29%
==========================================
Files 1040 1040
Lines 183023 183028 +5
==========================================
- Hits 155297 154782 -515
- Misses 27726 28246 +520 |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
Set timeout or body limit to 0 to disable them.
Checklist
Summary by CodeRabbit
http.timeoutto 0.http.body_limitto 0.