[#1195] Simplify Command Router HTTP query execution - #1214
[#1195] Simplify Command Router HTTP query execution#1214marcocapozzoli wants to merge 11 commits into
Conversation
… dropping max_concurrent_executions HTTP queries now build proxies from request params without peer-store sync, and parallelism is bounded only by thread_pool_size with max_queued_executions as in-flight backpressure.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe HTTP API now accepts ChangesHTTP Query Router
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CommandRouterHttpAPI
participant HttpCommandProxyFactory
participant ProxyParametersFromJson
participant BusCommandRouterProcessor
Client->>CommandRouterHttpAPI: Submit command and params
CommandRouterHttpAPI->>HttpCommandProxyFactory: Create query proxy
HttpCommandProxyFactory->>ProxyParametersFromJson: Set proxy properties
CommandRouterHttpAPI->>BusCommandRouterProcessor: Dispatch query with copied parameters
BusCommandRouterProcessor-->>CommandRouterHttpAPI: Publish execution events
CommandRouterHttpAPI-->>Client: Return status and answer envelopes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
marcocapozzoli
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
🧹 Nitpick comments (7)
src/agents/command_router/http_api/CommandExecution.h (1)
38-41: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider taking
paramsby value to allow a move.The constructor copies the whole
paramsobject. The HTTP layer parses the request body and does not need it afterwards. A by-value parameter plusstd::moveremoves one deep copy of the JSON tree per execution.♻️ Proposed signature change
CommandExecution(const string& execution_id, const string& command, - const json& params, + json params, size_t max_events = DEFAULT_MAX_EVENTS);Then in
CommandExecution.cc:CommandExecution::CommandExecution(const string& execution_id, const string& command, json params, size_t max_events) : execution_id(execution_id), command(command), params(std::move(params)), max_events(max_events) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandExecution.h` around lines 38 - 41, Update the CommandExecution constructor declaration and definition to accept params by value, then move it into the member initializer for params. Preserve the existing behavior for callers that pass lvalues while allowing HTTP request-owned JSON objects to avoid an extra deep copy.src/tests/cpp/command_router_http_api_test.cc (1)
815-832: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: exit the polling loop early.
The loop always runs all 100 attempts, so it issues 400 HTTP requests and sleeps about 5 seconds even after every execution reaches a terminal state. Break once
max_observed_runningreacheskThreadPoolSizeand all executions are terminal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/command_router_http_api_test.cc` around lines 815 - 832, Update the polling loop around max_observed_running to exit early once max_observed_running reaches kThreadPoolSize and every execution has reached a terminal state. Track or derive terminal completion while iterating execution_ids, preserve the existing assertions and final expectations, and retain the sleep only when another polling attempt is needed.src/agents/command_router/http_api/CommandExecution.cc (2)
197-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold the repeated status-envelope emission into one helper.
The same three-step sequence (build status params, add extras, publish envelope) now appears in
mark_running,mark_completed,mark_error,mark_aborted,mark_error_unless_terminal, andmark_aborted_unless_terminal. A single private helper keeps the envelope shape in one place, so a future field addition cannot miss one path.♻️ Sketch
void CommandExecution::publish_status_locked(json extra) { json params = this->status_params_locked(); for (auto& [key, value] : extra.items()) { params[key] = value; } this->publish_event_locked(this->make_envelope_locked(COMMAND_EXECUTION_STATUS, std::move(params))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandExecution.cc` around lines 197 - 210, Optionally add a private CommandExecution helper such as publish_status_locked to centralize status parameter construction, merging any extra fields, and COMMAND_EXECUTION_STATUS envelope publication. Replace the repeated emission sequences in mark_running, mark_completed, mark_error, mark_aborted, mark_error_unless_terminal, and mark_aborted_unless_terminal with this helper while preserving each method’s existing status updates and extras.
125-127: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the extra copy in object construction from the initializer list.
make_envelope_lockedcreates conststd::pairelements for the initializer list, sostd::move(params)only moves into that temporary pair.nlohmann::jsonthen copies the pair into the new object beforepublish_chunkdumps the envelope while still holdingmtx_. Build the envelope first, assignparams, and return it.♻️ Proposed fix
json CommandExecution::make_envelope_locked(const string& command, json params) const { - return {{"command", command}, {"params", std::move(params)}}; + json envelope = json::object(); + envelope["command"] = command; + envelope["params"] = std::move(params); + return envelope; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandExecution.cc` around lines 125 - 127, Update CommandExecution::make_envelope_locked to construct the JSON envelope first, then assign the moved params to its "params" member before returning it. Replace the initializer-list construction so std::move(params) transfers directly into the JSON object without the intermediate pair copy.src/agents/command_router/http_api/ProxyParametersFromJson.cc (1)
10-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd section banners to group the conversion helpers.
This file has six static methods and no section banners. The coding guideline for
.ccfiles undersrc/**/*.ccrequires grouping methods with banners like// ---.... Add a banner between the private per-type converters (set_boolthroughset_string) and the publicsetdispatcher.Based on coding guidelines ("Group C++ methods in
.ccfiles with section banners") and path instructions (".ccfiles use section banners like// ---...between API groups"), which apply to this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/ProxyParametersFromJson.cc` around lines 10 - 207, Add a section banner comment between the private conversion helpers ending with set_string and the public set dispatcher, using the repository’s established // ---... banner style. Do not alter the conversion logic or dispatcher behavior.Sources: Coding guidelines, Path instructions
src/agents/command_router/http_api/CommandRouterHttpAPI.cc (1)
120-147: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove
body["params"]instead of copying it.
bodyis not used again after line 128. Movebody["params"]into the localparamsvariable instead of copying it, to avoid a full JSON deep copy for every scheduled execution (this can matter whentokensarrays are large).Based on path instructions ("Flag unnecessary copies of large objects (Properties, vector, string, maps) — prefer const ref, std::move()"), which applies to this hot request-handling path.
♻️ Proposed fix to move the params JSON instead of copying it
const string command = body["command"].get<string>(); - const json params = body["params"]; + json params = std::move(body["params"]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc` around lines 120 - 147, Update the local params initialization in the command scheduling flow to move body["params"] rather than copy the JSON value, since body is not used afterward. Preserve the existing validation and pass the moved params to CommandExecution.Source: Path instructions
src/agents/command_router/BusCommandRouterProcessor.cc (1)
86-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMove
parametersintoprocessor_proxyfor the in-process dispatch path.
BusCommandRouterProxy::parametersis inherited fromBaseQueryProxy, not aBusCommandRouterProxyfield;poll_streamreadsparametersfrom the router proxy it receives. Useprocessor_proxy->parameters = std::move(caller_proxy->parameters)so the HTTP path avoids one redundant copy while keeping the streamed parameters populated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agents/command_router/BusCommandRouterProcessor.cc` around lines 86 - 90, Update the in-process dispatch assignments in BusCommandRouterProcessor to move parameters from caller_proxy into processor_proxy, matching the existing command and args moves. Preserve the populated parameters state consumed by poll_stream while avoiding the redundant copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tests/cpp/command_router_http_api_test.cc`:
- Around line 380-383: Make static initialization consistent in the HTTP
command-router tests: ensure CommandRouterHttpClient is registered before port
allocation while retaining BUS_COMMAND_ROUTER and the 49400–49499 range. Update
initialize_test_service_bus_statics_once or the surrounding
initialize_http_command setup, or initialize both command types together before
these tests run.
In `@src/tests/scripts/command_router_http_client.py`:
- Around line 76-87: Update the query request configuration in the
command-router HTTP client test so the cancellation scenario remains valid:
increase max_answers above 1 to keep execution running long enough for
cancellation, or adjust the later status assertions and wait_for_status call to
accept both aborted and completed terminal outcomes.
---
Nitpick comments:
In `@src/agents/command_router/BusCommandRouterProcessor.cc`:
- Around line 86-90: Update the in-process dispatch assignments in
BusCommandRouterProcessor to move parameters from caller_proxy into
processor_proxy, matching the existing command and args moves. Preserve the
populated parameters state consumed by poll_stream while avoiding the redundant
copy.
In `@src/agents/command_router/http_api/CommandExecution.cc`:
- Around line 197-210: Optionally add a private CommandExecution helper such as
publish_status_locked to centralize status parameter construction, merging any
extra fields, and COMMAND_EXECUTION_STATUS envelope publication. Replace the
repeated emission sequences in mark_running, mark_completed, mark_error,
mark_aborted, mark_error_unless_terminal, and mark_aborted_unless_terminal with
this helper while preserving each method’s existing status updates and extras.
- Around line 125-127: Update CommandExecution::make_envelope_locked to
construct the JSON envelope first, then assign the moved params to its "params"
member before returning it. Replace the initializer-list construction so
std::move(params) transfers directly into the JSON object without the
intermediate pair copy.
In `@src/agents/command_router/http_api/CommandExecution.h`:
- Around line 38-41: Update the CommandExecution constructor declaration and
definition to accept params by value, then move it into the member initializer
for params. Preserve the existing behavior for callers that pass lvalues while
allowing HTTP request-owned JSON objects to avoid an extra deep copy.
In `@src/agents/command_router/http_api/CommandRouterHttpAPI.cc`:
- Around line 120-147: Update the local params initialization in the command
scheduling flow to move body["params"] rather than copy the JSON value, since
body is not used afterward. Preserve the existing validation and pass the moved
params to CommandExecution.
In `@src/agents/command_router/http_api/ProxyParametersFromJson.cc`:
- Around line 10-207: Add a section banner comment between the private
conversion helpers ending with set_string and the public set dispatcher, using
the repository’s established // ---... banner style. Do not alter the conversion
logic or dispatcher behavior.
In `@src/tests/cpp/command_router_http_api_test.cc`:
- Around line 815-832: Update the polling loop around max_observed_running to
exit early once max_observed_running reaches kThreadPoolSize and every execution
has reached a terminal state. Track or derive terminal completion while
iterating execution_ids, preserve the existing assertions and final
expectations, and retain the sleep only when another polling attempt is needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f496abbe-9d1d-48a5-a416-7d9b6f730b64
📒 Files selected for processing (16)
src/agents/command_router/BusCommandRouterProcessor.ccsrc/agents/command_router/BusCommandRouterProcessor.hsrc/agents/command_router/http_api/BUILDsrc/agents/command_router/http_api/CommandExecution.ccsrc/agents/command_router/http_api/CommandExecution.hsrc/agents/command_router/http_api/CommandRouterHttpAPI.ccsrc/agents/command_router/http_api/CommandRouterHttpAPI.hsrc/agents/command_router/http_api/CommandRouterHttpAPIConfig.ccsrc/agents/command_router/http_api/CommandRouterHttpAPIConfig.hsrc/agents/command_router/http_api/HttpCommandProxyFactory.ccsrc/agents/command_router/http_api/HttpCommandProxyFactory.hsrc/agents/command_router/http_api/ProxyParametersFromJson.ccsrc/agents/command_router/http_api/ProxyParametersFromJson.hsrc/tests/cpp/BUILDsrc/tests/cpp/command_router_http_api_test.ccsrc/tests/scripts/command_router_http_client.py
💤 Files with no reviewable changes (1)
- src/agents/command_router/http_api/CommandRouterHttpAPIConfig.cc
1 participant
Summary
Simplifies how the Command Router HTTP API builds and runs
queryexecutions.Changes
HttpCommandProxyFactory/ProxyParametersFromJson, applying request params directly onto that proxy.max_concurrent_executions; concurrency is bounded only bythread_pool_size.