TykTechnologies
Add support for MaxConnsPerHost transport config
User description
Description
Related Issue
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality)
Checklist
- [ ] I ensured that the documentation is up to date
- [ ] I explained why this PR updates go.mod in detail with reasoning why it's required
- [ ] I would like a code coverage CI quality gate exception and have explained why
PR Type
Enhancement
Description
- Added a new configuration option
MaxConnsPerHostto theConfigstruct inconfig/config.go. - Documented the new
MaxConnsPerHostfield to explain its purpose and usage. - Integrated the
MaxConnsPerHostconfiguration into thehttp.Transportsetup ingateway/reverse_proxy.go.
Changes walkthrough 📝
| Relevant files | |||||
|---|---|---|---|---|---|
| Enhancement |
|
💡 PR-Agent usage: Comment
/helpon the PR to get a list of all available PR-Agent tools and their descriptions
Apply Sweep Rules to your PR?
- [ ] Apply: Leftover TODOs in the code should be handled.
- [ ] Apply: All new business logic should have corresponding unit tests in the tests/ directory.
- [ ] Apply: Any clearly inefficient or repeated code should be optimized or refactored.
![sweep-ai-deprecated[bot] avatar](https://avatars.githubusercontent.com/in/307814?v=4 "Published by a pub.dev verified publisher"){kind=small}
API Changes
--- prev.txt 2024-06-04 21:06:20.620549961 +0000
+++ current.txt 2024-06-04 21:06:17.668526964 +0000
@@ -5289,6 +5289,9 @@
MaxIdleConns int `bson:"max_idle_connections" json:"max_idle_connections"`
// Maximum idle connections, per API, per upstream, between Tyk and Upstream. Default:100
MaxIdleConnsPerHost int `bson:"max_idle_connections_per_host" json:"max_idle_connections_per_host"`
+ // Limits the total number of connections per host, including connections in the dialing, active and idele states.
+ // On limit violations, dials will block.
+ MaxConnsPerHost int `bson:"max_connections_per_host" json:"max_connections_per_host"`
// Maximum connection time. If set it will force gateway reconnect to the upstream.
MaxConnTime int64 `json:"max_conn_time"`
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
PR Analysis
- 🎯 Main theme: Adding support for MaxConnsPerHost transport configuration
- 📝 PR summary: This PR introduces a new configuration option, MaxConnsPerHost, which limits the total number of connections per host. This includes connections in the dialing, active, and idle states. When the limit is violated, dials will block.
- 📌 Type of PR: Enhancement
- 🧪 Relevant tests added: No
- ⏱️ Estimated effort to review [1-5]: 2, the PR is straightforward and introduces a new configuration option. However, it lacks tests to verify the new functionality.
- 🔒 Security concerns: No
PR Feedback
-
💡 General suggestions: The PR is generally well done, but it lacks tests for the new functionality. It would be beneficial to add tests that verify the behavior when the maximum number of connections per host is reached.
-
🤖 Code feedback:
-
relevant file:
config/config.gosuggestion: Consider adding a default value for MaxConnsPerHost in the case it is not provided in the configuration. This will prevent potential issues with uninitialized values. [important] relevant line: MaxConnsPerHost intbson:"max_connections_per_host" json:"max_connections_per_host" -
relevant file:
gateway/reverse_proxy.gosuggestion: It would be useful to handle the case where MaxConnsPerHost limit is reached and dials are blocked. This could be done by adding a timeout or a custom error message to inform the user about the situation. [medium] relevant line: MaxConnsPerHost: p.Gw.GetConfig().MaxConnsPerHost, // default is 0
-
How to use
To invoke the PR-Agent, add a comment using one of the following commands: /review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option. /describe: Modify the PR title and description based on the contents of the PR. /improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback. /ask <QUESTION>: Pose a question about the PR. /update_changelog: Update the changelog based on the PR's contents.
To edit any configuration parameter from configuration.toml, add --config_path=new_value For example: /review --pr_reviewer.extra_instructions="focus on the file: ..." To list the possible configuration parameters, use the /config command.
API tests result: success :white_check_mark:
Branch used: refs/pull/5690/merge
Commit:
Triggered by: pull_request (@zalbiraw)
Execution page
PR Review 🔍
| ⏱️ Estimated effort to review [1-5] |
2, because the changes are straightforward and localized to specific configuration parameters and their usage in the transport setup. The PR modifies existing structures and functions with new configuration options, which are easy to understand and review. |
| 🧪 Relevant tests |
No |
| ⚡ Possible issues |
No |
| 🔒 Security concerns |
No |
Code feedback:
| relevant file | config/config.go |
| suggestion |
Consider adding a default value for |
| relevant line | MaxConnsPerHost int `bson:"max_connections_per_host" json:"max_connections_per_host"` |
| relevant file | gateway/reverse_proxy.go |
| suggestion |
Ensure that setting |
| relevant line |
PR Code Suggestions ✨
| Category | Suggestion | Score |
| Maintainability |
Correct a typo in the commentCorrect the typo in the comment for Suggestion importance[1-10]: 10Why: Correcting typos in comments improves code readability and maintainability. The typo "idele" should be corrected to "idle" to ensure the comment is clear and professional. | 10 |
Add a comment to document the default behavior of
| 7 | |
| Possible issue |
Ensure
| 9 |
| Enhancement |
Add a default value comment for
| 7 |