feature: proto transport #200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
WalkthroughThe changes encompass a broad update to the handling of HTTP requests and responses, with a shift towards using protobuf for structured data handling and improved efficiency in multipart form data parsing. Additionally, updates to attribute handling and pool management aim to enhance performance and reliability. Changes
Assessment against linked issues
Possibly related issues
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 (
|
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (8)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sumtests/php_test_files/composer.jsonis excluded by!**/*.jsontests/php_test_files/composer.lockis excluded by!**/*.lock
Files selected for processing (20)
- .gitignore (1 hunks)
- .golangci.yml (1 hunks)
- attributes/attributes.go (4 hunks)
- handler/convert.go (1 hunks)
- handler/handler.go (5 hunks)
- handler/parse.go (4 hunks)
- handler/parse_test.go (2 hunks)
- handler/pool.go (1 hunks)
- handler/request.go (6 hunks)
- handler/response.go (4 hunks)
- handler/uploads.go (2 hunks)
- tests/attributes_test.go (3 hunks)
- tests/configs/.rr-http-init.yaml (1 hunks)
- tests/handler_test.go (11 hunks)
- tests/http_otlp_test.go (2 hunks)
- tests/http_plugin2_test.go (10 hunks)
- tests/http_plugin3_test.go (14 hunks)
- tests/http_plugin_test.go (34 hunks)
- tests/test_plugins/plugin1.go (1 hunks)
- tests/test_plugins/plugin_middleware.go (1 hunks)
Files skipped from review due to trivial changes (2)
- .gitignore
- .golangci.yml
Additional comments (88)
tests/test_plugins/plugin1.go (1)
- 1-1: Adding the
revivedirective to thenolintcomment is a good practice for ensuring code quality and consistency. It's important to ensure that all team members are aware of this change and adjust their local linter configurations accordingly.tests/configs/.rr-http-init.yaml (1)
- 20-35: Removing SSL, FastCGI, and HTTP/2 settings from the configuration file could have significant impacts on existing deployments that rely on these features. Ensure that these changes are clearly communicated in the release notes and documentation to avoid unexpected disruptions for users. Additionally, updating the log level to
debugmay increase the verbosity of logs, which could be beneficial for development but might need reconsideration for production environments.tests/test_plugins/plugin_middleware.go (1)
- 1-1: Adding the
revivedirective to thenolintcomment is consistent with the changes inplugin1.go. This consistency across the project is beneficial for maintaining code quality. Ensure that the development team is aware of these linter configuration changes.handler/convert.go (3)
- 10-24: The
convertfunction efficiently converts HTTP headers to protobuf structures. It's well-implemented and follows best practices for handling nil maps and pre-allocating the response map based on the size of the input. This should provide good performance characteristics.- 27-42: The
convertCookiesfunction mirrors the logic of theconvertfunction but for cookies. It's correctly implemented and adheres to best practices. Pre-allocating the response map is a good performance optimization.- 45-59: The
convertMimeHeaderfunction is similar to theconvertfunction, handling the conversion of MIME headers. The implementation is correct and follows best practices, including handling nil maps and pre-allocating the response map. This consistency in implementation style across conversion functions is commendable.tests/attributes_test.go (3)
- 20-20: The modification to expect values as slices of strings in assertions is a good adjustment that aligns with the changes in attribute handling. This ensures that tests accurately reflect the new behavior.
- 27-27: Ensuring that tests handle cases with no attributes correctly is important. The adjustment to expect an empty map instead of a nil value or a different type is appropriate and improves test clarity.
- 33-33: The addition of a test case to explicitly check for nil values when no attributes are present is a good practice. It ensures that the behavior is consistent and predictable.
attributes/attributes.go (2)
- 18-29: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [11-26]
Changing the
attrstype tomap[string][]stringand modifying thesetmethod to append values to a slice instead of directly setting them are significant improvements. These changes enhance the flexibility and correctness of attribute handling, allowing for multiple values per key.
- 44-64: The modifications to the
Allfunction to handle different types in the context and convert them tomap[string][]stringare well thought out. This ensures compatibility and flexibility in handling attributes, accommodating various use cases.handler/response.go (3)
- 31-39: Switching to protobuf for handling responses and explicitly stating that the JSON codec is not supported are significant changes. Ensure that this shift is clearly communicated to users and developers, as it affects how responses are processed and may require adjustments in client code.
- 28-76: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [41-83]
The implementation of
handlePROTOresponseto process protobuf responses, including handling HTTP/2 push headers and trailers, is well-executed. It's important to ensure comprehensive testing around these features, especially the handling of push headers and trailers, as they can significantly affect client behavior.
- 102-109: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [105-115]
The handling of protobuf trailers in
handleProtoTrailersis a nuanced feature that enhances HTTP/2 support. Ensure that this functionality is covered by tests to prevent regressions and verify its behavior across different HTTP/2 implementations.handler/pool.go (2)
- 14-43: The implementation of request object pooling and conversion to protobuf structures in
getProtoReqandputProtoReqfunctions is efficient and follows best practices. Using sync pools for managing request objects can significantly reduce memory allocations and improve performance.- 82-115: The management of response objects and channels using sync pools in
putProtoRsp,getProtoRsp,putCh, andgetChfunctions is well-implemented. This approach optimizes resource usage and enhances performance, especially under high load. Ensure that the use of sync pools is consistently applied across the project for similar use cases.handler/uploads.go (1)
- 26-26: The alignment of the
patternvariable declaration seems to have been adjusted for consistency. This is a minor stylistic change that improves the readability of the code.handler/handler.go (4)
- 51-55: The introduction of new sync pools (
protoRespPoolandprotoReqPool) alongside adjustments to existing pools (reqPool,pldPool,stopChPool) is a significant change aimed at optimizing the handling of protocol-specific request and response objects. This change is likely to improve performance and resource management within the handler.- 85-85: The initialization of
Requestobjects within thereqPoolsync pool now includes the initialization ofAttributesas a map of string slices. This change aligns with the shift towards using protobuf for data transport, as it allows for more flexible handling of multiple values for the same attribute key.- 93-94: The initialization of
httpV1proto.Responseobjects within theprotoRespPoolsync pool now explicitly sets theStatusfield to-1. This change seems to indicate an uninitialized or error state by default, which could be a way to signal that the response status needs to be explicitly set later in the processing flow.- 153-156: The retrieval of a proto request object from the
protoReqPooland its subsequent use in thePayloadfunction represents a key part of the changes aimed at leveraging protobuf for data transport. This approach is expected to enhance the efficiency and reliability of data handling.handler/parse.go (2)
- 48-53: The modification to the
pushmethod in bothdataTreeandfileTreeto use a new approach for handling keys, including the use of thefetchIndexesfunction with a pointer to keys, represents an improvement in the way multipart data is parsed and organized. This change is likely to enhance the efficiency and reliability of multipart data handling.- 261-276: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [246-272]
The changes to the
fetchIndexesfunction, including the addition of akeysparameter passed by reference, update the logic for parsing input names into separate indexes. This approach allows for more direct manipulation of the keys array and is a key part of the improvements in multipart data parsing.tests/http_otlp_test.go (2)
- 38-38: Updating the version field in the
config.Pluginstruct from "2.10.0" to "2023.3.5" in theTestHTTPOTLP_Initfunction reflects an update to the test configuration to align with the new versioning scheme. This change ensures that the tests are running against the intended version of the plugin.- 135-135: Similarly, updating the version field in the
config.Pluginstruct in theTestHTTPOTLP_WithPHPfunction to "2023.3.5" aligns the test configuration with the new versioning scheme. This consistency in versioning across tests is crucial for maintaining the accuracy and relevance of the test suite.handler/request.go (5)
- 4-4: The addition of the
encoding/jsonimport is necessary for marshalingUploadsinto JSON. This change supports the new functionality introduced in thePayloadfunction for handlinghttpV1proto.Requestandproto.Marshal.- 48-48: Changing the type of
Attributesfrommap[string]anytomap[string][]stringin theRequeststruct aligns with the shift towards using protobuf for data transport. This change allows for more flexible handling of multiple values for the same attribute key, enhancing the capability to pass complex data structures from Golang to PHP.- 93-101: The logic for handling request bodies in the
requestfunction, particularly for multipart content whensendRawBodyis true, has been updated to read the entire body intoreq.body. This change addresses the need to support raw body content for multipart requests, improving compatibility with protobuf and the handling of larger payloads.- 168-232: The updates to the
Payloadfunction, including handlinghttpV1proto.Requestandproto.Marshal, as well as the logic for marshalingr.Uploadsand handling different types of request bodies, represent a comprehensive enhancement to the request handling process. These changes are crucial for leveraging protobuf for data transport and ensuring efficient handling of form data and file uploads.- 273-286: The addition of the
packDataTreefunction for packing data trees into the payload body is a necessary enhancement to support the structured representation of form data. This function facilitates the conversion of complex data structures into a format suitable for transport, further enhancing the flexibility and efficiency of data handling.handler/parse_test.go (2)
- 29-32: The modifications to the
Test_FetchIndexesfunction to use a new approach wherefetchIndexesnow takes an additional parameterkeyspassed by reference, updating the logic to compare the fetched keys with the expected output, ensure that the tests accurately reflect the updated functionality of thefetchIndexesfunction.- 41-43: Similarly, the changes to the
BenchmarkConfig_FetchIndexesfunction to adapt to the new approach of passingkeysby reference to thefetchIndexesfunction ensure that the benchmark tests accurately measure the performance of the updatedfetchIndexeslogic.tests/http_plugin2_test.go (9)
- 5-5: The import for
contexthas been added to support the use ofhttp.NewRequestWithContextin HTTP requests. This is a good practice as it allows for better control over request cancellation and deadlines.- 41-41: Updating the
Versionfield inconfig.Plugininstances to "2023.3.5" is appropriate for keeping the configuration up to date with the latest version. Ensure that this version is compatible with all dependencies and does not introduce breaking changes.- 121-124: Switching to
http.NewRequestWithContextfor creating HTTP requests is a good practice as it allows passing a context that can be used to cancel the request or set deadlines. This change enhances the robustness of the test functions.- 139-139: Adding the "Content-Type" header to the request is crucial for informing the server about the type of the data being sent. This is especially important for APIs that expect a specific content type.
- 158-158: The repetition of updating the
Versionfield inconfig.Plugininstances across different test functions suggests a pattern of keeping the plugin configuration up to date. As before, ensure compatibility with this version.- 260-260: The update to the
Versionfield in the context of testing the file server functionality follows the same rationale as previous instances. Consistency in versioning across tests is key for reliable test outcomes.- 354-354: Again, updating the
Versionfield in the context of testing large responses. It's good to see consistency in maintaining the plugin versions across different test scenarios.- 452-452: The update to the
Versionfield in the context of testing HTTP execution TTL. This change is consistent with updates in other test functions, emphasizing the importance of keeping the plugin version current.- 536-536: The update to the
Versionfield in the context of testing HTTP big response with max request size. This is another instance where the plugin version is kept up to date, which is crucial for testing against the latest functionalities and fixes.tests/http_plugin3_test.go (11)
- 6-6: The addition of the
contextimport is consistent with the changes made to usehttp.NewRequestWithContextin HTTP requests. This is a positive change for managing request lifecycles more effectively.- 425-425: Updating the
Versionfield inconfig.Plugininstances to "2023.3.5" across various test functions ensures that the tests are run against the latest version of the plugin. This consistency is important for accurate testing.- 631-631: The update to the
Versionfield in the context of testing MTLS functionality is another instance of maintaining consistency in plugin versions across tests. This practice ensures that tests are relevant and up to date.- 728-728: Similar to previous comments, updating the
Versionfield in the context of another MTLS test function is a good practice for ensuring that tests are conducted against the current version of the plugin.- 821-821: The consistency in updating the
Versionfield inconfig.Plugininstances is observed here as well, in the context of MTLS testing. This consistency across test functions is commendable.- 914-914: Again, updating the
Versionfield in the context of MTLS testing. This change is part of a broader pattern of keeping the plugin version current across all test functions.- 973-973: Replacing
http.NewRequest("GET", ...)withhttp.NewRequest(http.MethodGet, ...)inTestMTLS4is a minor but good practice for consistency and readability. Using thehttp.MethodGetconstant makes the HTTP method explicit and avoids potential typos.- 1007-1007: The update to the
Versionfield in the context of another MTLS test function. This change, consistent with others, ensures that the tests are aligned with the latest plugin version.- 1088-1088: Updating the
Versionfield in the context of testing large URL-encoded bodies. This change is part of the consistent effort to keep the plugin version updated across all test scenarios.- 1178-1178: The update to the
Versionfield in the context of testing another scenario of large URL-encoded bodies. This consistency in versioning is important for ensuring that tests are relevant to the current plugin version.- 1269-1269: Updating the
Versionfield in the context of testing large URL-encoded bodies with a different configuration. This change is consistent with the practice of keeping the plugin version current across different test functions.tests/handler_test.go (5)
- 358-359: Using a custom HTTP client instead of
http.DefaultClientis a good practice, especially in tests, as it allows for more control over the request execution and can prevent issues related to shared state across tests.- 436-436: Specifying the HTTP method using
http.MethodPostimproves code readability and maintainability by using the standard library's constants instead of string literals.- 871-875: The use of
map[string]anyfor JSON unmarshalling and replacing[]interface{}with[]anyfor slice assertions are modern and cleaner approaches, aligning with Go's latest versions and best practices.- 1535-1542: Creating a new HTTP client for sending a PATCH request with multipart form data is consistent with the changes made in other tests. This ensures isolation between tests and allows for specific client configurations if needed.
- 1946-1953: The benchmark test setup and execution look correct. It's good to see the use of
http.NewRequestWithContextfor better control over request cancellation and timeouts. Also, the use of a custom HTTP client and the proper handling of the response body, including closing it, are best practices.tests/http_plugin_test.go (31)
- 52-52: The version number in the configuration settings has been updated from a previous version to "2023.3.5". This change is consistent across multiple test functions. Ensure that this version update aligns with the intended compatibility and features of the RoadRunner server.
- 117-117: Version update to "2023.3.5" in the HTTP access logs test configuration. Similar to previous comments, ensure this version is correct and tested.
- 201-201: Version update to "2023.3.5" in the HTTP X-Sendfile test configuration. Consistency in version updates across tests is good for maintaining compatibility.
- 292-292: Version update to "2023.3.5" in the HTTP no config section test. Consistent versioning across tests is noted.
- 356-356: Version update to "2023.3.5" in the HTTP informer reset test configuration. It's important to ensure that the new version is compatible with the features being tested.
- 431-431: Version update to "2023.3.5" in the SSL test configuration. As with other version updates, ensure compatibility and feature support with this new version.
- 592-592: Version update to "2023.3.5" in the SSL redirect test configuration. Consistency in version updates is good practice.
- 670-670: Switching from
http.NewRequesttohttp.NewRequestWithContextin the SSL redirect test. This change is part of the modernization of HTTP request creation, allowing for better context handling and cancellation capabilities.- 694-694: Version update to "2023.3.5" in the SSL push pipes test configuration. Ensure this version update is intended and tested for compatibility.
- 770-770: Switching from
http.NewRequesttohttp.NewRequestWithContextin the SSL push test. This is a positive change for modern HTTP request handling.- 797-797: Version update to "2023.3.5" in the FastCGI echo test configuration. Consistency in version updates across tests is noted.
- 883-883: Version update to "2023.3.5" in the FastCGI echo Unix test configuration. Ensure this version update aligns with the intended compatibility and features.
- 973-973: Version update to "2023.3.5" in the FastCGI request URI test configuration. As with other version updates, ensure compatibility and feature support with this new version.
- 1059-1059: Version update to "2023.3.5" in the HTTP/2 request test configuration. Consistency in version updates is good practice.
- 1159-1159: Version update to "2023.3.5" in the H2C upgrade test configuration. Ensure this version update is intended and tested for compatibility.
- 1220-1220: Switching from
http.NewRequesttohttp.NewRequestWithContextin the H2C upgrade test. This change is part of the modernization of HTTP request creation, allowing for better context handling and cancellation capabilities.- 1255-1255: Version update to "2023.3.5" in the H2C test configuration. Consistency in version updates across tests is noted.
- 1351-1351: Version update to "2023.3.5" in the HTTP middleware test configuration. Ensure this version update aligns with the intended compatibility and features.
- 1481-1481: Version update to "2023.3.5" in the HTTP echo error test configuration. As with other version updates, ensure compatibility and feature support with this new version.
- 1568-1568: Version update to "2023.3.5" in the HTTP broken pipes test configuration. Consistency in version updates is good practice.
- 1596-1596: Version update to "2023.3.5" in the HTTP supervised pool test configuration. Ensure this version update is intended and tested for compatibility.
- 1729-1729: Version update to "2023.3.5" in the HTTP big request size test configuration. Consistency in version updates across tests is noted.
- 1816-1816: Version update to "2023.3.5" in the static ETag plugin test configuration. Ensure this version update aligns with the intended compatibility and features.
- 1927-1927: Version update to "2023.3.5" in the static plugin security test configuration. As with other version updates, ensure compatibility and feature support with this new version.
- 2149-2149: Version update to "2023.3.5" in the static plugin test configuration. Consistency in version updates is good practice.
- 2269-2269: The configuration for the static disabled error test indicates an attempt to disable static file serving, which should result in an initialization error. This test is designed to verify that the server correctly handles the configuration to disable static file serving.
- 2290-2290: Version update to "2023.3.5" in the static files disabled test configuration. Ensure this version update is intended and tested for compatibility.
- 2368-2368: Version update to "2023.3.5" in the static files forbid test configuration. Consistency in version updates across tests is noted.
- 2469-2469: Version update to "2023.3.5" in the HTTP issue 659 test configuration. Ensure this version update aligns with the intended compatibility and features.
- 2538-2538: Version update to "2023.3.5" in the HTTP IPv6 long test configuration. As with other version updates, ensure compatibility and feature support with this new version.
- 2607-2607: Version update to "2023.3.5" in the HTTP IPv6 short test configuration. Consistency in version updates is good practice.
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
Files selected for processing (4)
- .github/workflows/linters.yml (1 hunks)
- .golangci.yml (3 hunks)
- handler/convert.go (1 hunks)
- tests/php_test_files/composer.json (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- handler/convert.go
Additional Context Used
Additional comments not posted (5)
.github/workflows/linters.yml (1)
21-21: Updating the linter version to v1.57 is a good practice to benefit from the latest features and fixes. Ensure to review the linter's release notes for any breaking changes or new rules that could impact the existing codebase.tests/php_test_files/composer.json (1)
2-25: The updates to dependency versions incomposer.jsonare crucial for maintaining compatibility and leveraging new features. It's recommended to thoroughly test these updates to ensure they do not introduce any breaking changes or incompatibilities with the existing test suite..golangci.yml (3)
8-8: The change to the output format toformats: colored-line-numberis a minor but helpful adjustment for readability.
14-14: Modifying thegovetsetting toshadow: truealigns with best practices for identifying shadowed variables, which can prevent subtle bugs.
78-80: Ensure that the removal of thegocognitlinter is intentional. While simplifying the linting process can be beneficial, it's important to balance this with the need for thorough code analysis to maintain quality.
|
Oh my god it's happening!! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reason for This PR
closes: roadrunner-server/roadrunner#1840
closes: roadrunner-server/roadrunner#1859
closes: roadrunner-server/roadrunner#1696
closes: roadrunner-server/roadrunner#1812
closes: roadrunner-server/roadrunner#1634
closes: roadrunner-server/roadrunner#1698
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
Summary by CodeRabbit
.gitignoreto ignore specific test files and directories.