Backport PR #2872 to release/v1.7 for implement bidirectional_stream #2880
Conversation
* implement bidirectional_stream * fix * add network test * add network test with duration * add CI * fix test command
Deploying vald with
|
| Latest commit: |
9eaebcd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://86c6fc6e.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-featur-wepr.vald.pages.dev |
📝 WalkthroughWalkthroughThis pull request updates the continuous integration workflows and testing infrastructure for Rust components. The changes include renaming an existing test step for the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant bidirectional_stream
participant TokioChannel
participant ProcessingFunction
Client->>bidirectional_stream: Send request stream
bidirectional_stream->>TokioChannel: Buffer incoming requests
loop Process up to concurrency limit
TokioChannel->>ProcessingFunction: Process individual request
ProcessingFunction-->>TokioChannel: Return result/error
end
TokioChannel->>Client: Send aggregated responses
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rust/bin/agent/src/handler/common.rs (2)
31-84: Consider a more robust concurrency approach.
Using an array of spawned tasks and manually popping handles works, but you might benefit fromtokio::sync::Semaphoreor aFuturesUnorderedapproach for more fine-grained parallelism and earlier completion.
86-356: Comprehensive test coverage is commendable!
These tests thoroughly validate normal and error scenarios, plus network integration. A further nitpick is to include a concurrency=1 or zero-message scenario for edge case coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/unit-test-rust.yaml(1 hunks)Makefile.d/test.mk(2 hunks)rust/bin/agent/Cargo.toml(2 hunks)rust/bin/agent/src/handler/common.rs(1 hunks)
🔇 Additional comments (8)
rust/bin/agent/Cargo.toml (2)
30-30: Nice addition of thefuturesdependency.
This version should integrate well with Tokio for async handling.
39-42:❓ Verification inconclusive
Check for up-to-date and secure dev dependencies.
It's always a good idea to verify thatbytesandhttp-bodyare on stable, vulnerability-free releases.Below is a script to run a preliminary security check on these crates:
🏁 Script executed:
#!/bin/bash # Description: Use cargo-audit to check for vulnerabilities in newly added dependencies. # Ensure cargo-audit is installed cargo install cargo-audit || echo "cargo-audit already installed or failed to install; continuing..." # Run cargo-audit in the agent manifest directory cd rust/bin/agent cargo auditLength of output: 394
Dependency Security Check Advisory
The preliminary audit didn't run successfully due to the lack of a configured Rust environment (i.e.
cargowas not found). Please verify in an environment where Rust and Cargo are available that the versions listed forbytes(1.10.0) andhttp-body(1.0.1) have no known vulnerabilities.
- Confirm that Cargo is installed and accessible.
- Re-run
cargo auditin the appropriate environment to ensure these dev-dependencies are secure.Makefile.d/test.mk (2)
293-298: Grouping Rust tests together is a clean organization.
Invokingtest/rust/qbgandtest/rust/agentundertest/rustkeeps the workflow consistent.
309-313:❓ Verification inconclusive
Verify coverage of all Agent tests.
Limiting tests tohandler::common::testsmay skip other tests if present in the agent package. Please verify that you’re not missing any other test modules.Below is a script that searches for other test attributes within the agent package:
🏁 Script executed:
#!/bin/bash # Description: Search for all test definitions to ensure none are missed in the new Make target. rg '#\[test\]' rust/bin/agent/src/ -A 5Length of output: 41
Ensure complete Agent test coverage manually.
The automated search for
#[test]attributes in therust/bin/agent/src/directory did not return any additional tests beyond those inhandler::common::tests. However, since the search command produced no output—which might be due to tests being defined in other directories or modules—it’s recommended that you manually verify the agent package for any extra test definitions.
- Revisit the agent package’s directory structure in case tests are located outside of
rust/bin/agent/src/.- Confirm that there are no additional test modules that need to be included in the Make target.
.github/workflows/unit-test-rust.yaml (2)
49-49: Renaming the test step provides clarity.
Switching from "Run tests for Rust / gotestfmt" to "Run tests for qbg / gotestfmt" makes it more explicit.
52-54: Adding a dedicated step for agent testing is well-structured.
This ensures each Rust component is tested individually and reported clearly.rust/bin/agent/src/handler/common.rs (2)
16-23: Imports look appropriate for async streaming.
These crates align well with the bidirectional streaming logic.
27-27: Simplifying thestream_type!macro.
Switching toReceiverStreamcan improve readability and reduce complexity compared to pinned streams.
|
[CHATOPS:HELP] ChatOps commands.
|
…mplement-bidirectional-stream2
Description
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
Chores