agents: DRY DelegateAsyncExport to use templates#294
agents: DRY DelegateAsyncExport to use templates#294santigimeno wants to merge 1 commit intonode-v22.x-nsolid-v5.xfrom
Conversation
WalkthroughThe changes refactor the gRPC asynchronous export mechanism for various event types in the codebase. The implementation of the templated asynchronous call handler ( Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)agents/grpc/src/grpc_client.h (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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: 1
🔭 Outside diff range comments (1)
agents/grpc/src/grpc_agent.cc (1)
1748-1757:⚠️ Potential issuePass arena‑allocated pointer, not a copy
Each of these call‑sites dereferences the arena‑allocated message and passes
std::move(*event)intoDelegateAsyncExport.
After refactor (and to fix the dangling‑pointer problem highlighted ingrpc_client.h) you should pass the pointer itself:- std::move(context), std::move(arena), std::move(*event), + std::move(context), std::move(arena), event,No copy is performed, lifetime is tied to the arena that
call_dataalready owns, and the template signature becomes:DelegateAsyncExport<grpcagent::BlockedLoopEvent>( stub, &grpc_async::ExportBlockedLoop, std::move(ctx), std::move(arena), event, cb);Search‑and‑replace for all nine similar occurrences shown in this hunk.
Also applies to: 1797-1800, 1841-1844, 1869-1872, 1892-1895, 1916-1919, 1967-1970, 1990-1993, 2015-2018
🧹 Nitpick comments (3)
agents/grpc/src/grpc_client.h (1)
98-111: Prefer RAII over rawnew/deleteforevent_response
event_responseis heap‑allocated withnewand manually deleted after the user callback.
Consider wrapping it in astd::unique_ptr<grpcagent::EventResponse>(or store it directly insideGrpcAsyncCallData) to make ownership explicit and exception‑safe.- grpcagent::EventResponse* event_response = nullptr; + std::unique_ptr<grpcagent::EventResponse> event_response;and adjust the lambda to pass
event_response.get().A minor change, but it eliminates a fragile manual delete.
agents/grpc/src/grpc_agent.cc (1)
53-54: Alias name can be more explicit
using grpc_async = ...::async_interface;shortens the code nicely, but the very generic namegrpc_asynccan clash in wider scopes.
Consider something likeNSolidAsyncStuborGrpcNSolidAsyncto make its provenance clear.agents/grpc/src/grpc_client.cc (1)
82-83: Remove stale comments once refactor stabilisesGood to keep a migration note during review, but once the header houses the templates permanently these comments can be pruned to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
agents/grpc/src/grpc_agent.cc(10 hunks)agents/grpc/src/grpc_client.cc(1 hunks)agents/grpc/src/grpc_client.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-js-and-md
- GitHub Check: build-docs
- GitHub Check: coverage-windows
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/build-tarball.yml (1)
108-108: Simplify TEST_CI_ARGS and remove extra spacing
Removing the deprecated--node-argsflag and condensing the argument string.Apply this minimal diff to clean up the double space:
- TEST_CI_ARGS="-p dots --measure-flakiness 9" + TEST_CI_ARGS="-p dots --measure-flakiness 9".github/workflows/coverage-linux.yml (1)
73-74: Simplify TEST_CI_ARGS and tolerate inspector-related failures
Removed the explicit--node-argsoption and added|| exit 0to let coverage collection proceed despite known inspector test failures.Consider trimming the extra space:
- TEST_CI_ARGS="-p dots --measure-flakiness 9" + TEST_CI_ARGS="-p dots --measure-flakiness 9".github/workflows/coverage-linux-without-intl.yml (1)
73-74: Simplify TEST_CI_ARGS and allow coverage step to continue
Removed obsolete--node-argsand ensured failures don’t block coverage upload.You may also trim the double space:
- TEST_CI_ARGS="-p dots --measure-flakiness 9" + TEST_CI_ARGS="-p dots --measure-flakiness 9"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/build-tarball.yml(4 hunks).github/workflows/coverage-linux-without-intl.yml(2 hunks).github/workflows/coverage-linux.yml(2 hunks).github/workflows/test-linux.yml(1 hunks).github/workflows/test-macos.yml(5 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test-linux.yml
42-42: label "ubuntu-24.04-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-tarball-linux
- GitHub Check: coverage-windows
- GitHub Check: coverage-linux
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: coverage-linux-without-intl
- GitHub Check: test-macOS
- GitHub Check: test-linux (ubuntu-24.04)
🔇 Additional comments (32)
.github/workflows/build-tarball.yml (5)
49-49: Upgrade to actions/setup-python v5.4.0
Bumping to the latest patch release for setup-python ensures compatibility with Python 3.12.
53-53: Upgrade sccache-action to v0.0.8
The newer version includes bugfixes and performance improvements for caching compilation artifacts.
55-55: Bump sccache version to v0.10.0
Updating to the latest sccache release to leverage improved cache backend support.
67-67: Upgrade upload-artifact to v4.6.1
Aligns with other workflows and pulls in incremental fixes.
89-89: Upgrade download-artifact to v4.1.9
Keeps artifact download in sync with upload step and addresses recent stability patches..github/workflows/coverage-linux.yml (5)
57-57: Upgrade to actions/setup-python v5.4.0
Consistent with other CI workflows; ensures Python 3.12 support.
61-61: Upgrade sccache-action to v0.0.8
Brings in the latest cache improvements for C/C++ builds.
63-63: Bump sccache version to v0.10.0
Matches the version used in other workflows for consistency.
75-77: Add JavaScript coverage reporting step
Runningnpx c8 report --check-coveragewith increased memory helps enforce JS coverage thresholds.
84-84: Upgrade codecov-action to v5.4.0
Ensures compatibility with the latest Codecov API and reporting features..github/workflows/coverage-linux-without-intl.yml (5)
57-57: Upgrade to actions/setup-python v5.4.0
Aligns with the main coverage workflow for Python 3.12 support.
61-61: Upgrade sccache-action to v0.0.8
Consistent cache behavior across workflows.
63-63: Bump sccache version to v0.10.0
Keeps sccache at the latest stable release.
75-77: Add JavaScript coverage reporting step
Usingnpx c8 report --check-coveragewith extra Node memory to enforce coverage rules.
84-84: Upgrade codecov-action to v5.4.0
Matches other workflows for consistent Codecov integration..github/workflows/test-linux.yml (7)
38-43: Verify availability of theubuntu-24.04-armrunner label
Dynamicruns-on: ${{ matrix.os }}is correct for matrix builds, but the labelubuntu-24.04-armmay not exist on GitHub-hosted runners. Confirm it’s provided via self-hosted or update the label if needed.🧰 Tools
🪛 actionlint (1.7.4)
42-42: label "ubuntu-24.04-arm" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
47-48: Clone repository intonode/subdirectory
Ensures isolation of Node-specific build and test steps.
49-51: Upgrade to actions/setup-python v5.4.0
Consistent with other CI workflows for Python 3.12.
53-55: Upgrade sccache-action to v0.0.8 & bump version to v0.10.0
Keeps caching aligned with other workflows.
59-59: Run build insidenode/directory
Correctly scopes the make invocation to the cloned subdirectory.
61-61: Simplify TEST_CI_ARGS for Linux tests
Removed deprecated flags and standardized on-p actions --measure-flakiness 9.
63-69: Add robustness test in directory with special characters
Rerunning tests in a path containing unusual characters helps catch path-encoding issues in the test harness..github/workflows/test-macos.yml (10)
33-33: Add XCODE_VERSION environment variable
Pinning Xcode to 16.1 ensures consistent build environment on macOS runners.
42-43: Disable fail-fast in matrix build
Allows both jobs to complete on failures, improving parallel feedback.
53-54: Clone repository intonode/subdirectory
Aligns with Linux CI structure for Node-based builds.
55-57: Upgrade to actions/setup-python v5.4.0
Standardizes Python setup across platforms.
58-60: Add Xcode toolchain selection step
Switches to the specified Xcode app path for builds and simulator runs.
61-63: Upgrade sccache-action to v0.0.8 & bump version to v0.10.0
Brings caching improvements to macOS build steps.
73-73: Pre-warmtools/doc/node_modulesto surface failures early
Good strategy to catch flakynpm cierrors before the main build.
89-89: Run build insidenode/directory on macOS
Correctly scopes the invocation to the cloned subdirectory.
93-93: Simplify TEST_CI_ARGS for macOS tests
Standardized on-p actions --measure-flakiness 9and removed legacy flags.
95-100: Add robustness test in directory with special characters
Mirrors Linux workflow to catch path-encoding issues on macOS.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
agents/grpc/src/grpc_agent.cc(10 hunks)agents/grpc/src/grpc_client.cc(0 hunks)agents/grpc/src/grpc_client.h(2 hunks)
💤 Files with no reviewable changes (1)
- agents/grpc/src/grpc_client.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- agents/grpc/src/grpc_agent.cc
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build-tarball
- GitHub Check: test-macOS
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-linux-without-intl
- GitHub Check: coverage-linux
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
agents/grpc/src/grpc_client.h (1)
4-8:⚠️ Potential issueMissing direct include for
google::protobuf::Arena
Arenais used viausing google::protobuf::Arena;but its header (google/protobuf/arena.h) is not included. Relying on transitive inclusion from generated protobuf headers is brittle and can break when those headers change.+#include <google/protobuf/arena.h>Add this include just above the existing protobuf/gRPC headers to ensure the file builds in isolation.
🧹 Nitpick comments (1)
agents/grpc/src/grpc_client.h (1)
93-97: Perfect‑forward the event to avoid an unnecessary move/copy
Arena::Createis called withstd::move(event), which turns the forwarding reference into an rvalue but still forces a copy or move of the whole protobuf message.
You can construct the message in‑place inside the arena and perfectly forward the constructor arguments, avoiding one extra move and keeping the code generic for non‑movable types:- call_data->event = Arena::Create<EventT>(call_data->arena.get(), - std::move(event)); + call_data->event = Arena::Create<EventT>(call_data->arena.get(), + std::forward<EventT>(event));This is a small optimization but aligns with modern C++ best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
agents/grpc/src/grpc_client.h (2)
42-43: Consider using= defaultfor special member functions.The constructor and destructor are empty but use manual definition. Modern C++ practice favors
= defaultfor clearer intent and potential compiler optimizations.- GrpcAsyncCallData() {} - ~GrpcAsyncCallData() {} + GrpcAsyncCallData() = default; + ~GrpcAsyncCallData() = default;
27-44: Consider adding rule-of-five special member functions toGrpcAsyncCallData.The class manages resources (unique_ptrs and function objects) but doesn't declare copy/move constructors and assignment operators. Consider either explicitly deleting them or properly implementing them to prevent unintended behavior.
public: std::unique_ptr<google::protobuf::Arena> arena; ::grpc::Status grpc_status; std::unique_ptr<::grpc::ClientContext> grpc_context; std::function<bool(::grpc::Status, std::unique_ptr<google::protobuf::Arena>&&, const EventType&, grpcagent::EventResponse*)> result_callback; EventType* event = nullptr; grpcagent::EventResponse* event_response = nullptr; GrpcAsyncCallData() = default; ~GrpcAsyncCallData() = default; + + // Disable copying because of unique_ptr members + GrpcAsyncCallData(const GrpcAsyncCallData&) = delete; + GrpcAsyncCallData& operator=(const GrpcAsyncCallData&) = delete; + + // Enable moving + GrpcAsyncCallData(GrpcAsyncCallData&&) = default; + GrpcAsyncCallData& operator=(GrpcAsyncCallData&&) = default;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
agents/grpc/src/grpc_client.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-macOS
- GitHub Check: coverage-linux
- GitHub Check: build-tarball
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
🔇 Additional comments (3)
agents/grpc/src/grpc_client.h (3)
87-101: The event parameter handling looks good now.The previous issue with dangling pointers has been fixed. The new code correctly creates a copy of the event in the arena, ensuring it outlives the asynchronous call, and proper error handling is implemented.
I also see that the
noexceptspecifier has been removed, addressing the concern about exceptions potentially terminating the process during allocation failures.
109-115: The callback handling looks improved.The previous issue with memory deallocation has been fixed. The code no longer attempts to delete memory allocated by the arena.
However, there's a potential thread safety consideration:
call_data->result_callback(call_data->grpc_status, std::move(call_data->arena), *call_data->event, call_data->event_response);Ensure that the
result_callbackfunction is thread-safe, as it might be invoked from a different thread than the one that created it, and it's working with potentially shared data.
66-118:Details
✅ Verification successful
The templated approach to
DelegateAsyncExportis a good improvement.This refactoring successfully applies the DRY principle by replacing multiple event-specific overloads with a single templated version. The implementation is robust and handles resource management appropriately:
- It creates the event in the arena to ensure it outlives the function call
- It properly handles error cases and returns an appropriate error code
- It sets up the callback with a lambda that properly manages the shared state
One thing to verify: does this templated implementation maintain backward compatibility with all existing call sites? The documentation example at lines 69-75 shows how to use it, but it would be good to confirm that all callers have been updated correctly.
🏁 Script executed:
#!/bin/bash # Check for any remaining calls to DelegateAsyncExport that might not be updated to use the new template parameter rg -A 3 "DelegateAsyncExport\(" --type cpp | grep -v "async_interface"Length of output: 7427
Verified: No internal call sites require updating
A search for
DelegateAsyncExport(within ouragents/directory only returned its declaration inagents/grpc/src/grpc_client.h. All external occurrences indeps/opentelemetry-cppreference theOtlpGrpcClient::DelegateAsyncExportoverloads in that module—not our new templated function—so they’re unaffected.Backward compatibility is maintained; no further changes are needed.
santigimeno
force-pushed
the
santi/dry_grpc_client
branch
from
e534f5f to
b897424
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
agents/grpc/src/grpc_client.h (2)
27-44: Explicitly disable copying/moving ofGrpcAsyncCallData
GrpcAsyncCallDataowns anstd::unique_ptr<Arena>and raw pointers that point into that arena.
Although the implicitly‑generated copy/move ctors are already deleted by the presence of theunique_ptr, making that fact explicit documents the intent and prevents accidental refactors that might re‑enable copying later.GrpcAsyncCallData() = default; ~GrpcAsyncCallData() = default; + + GrpcAsyncCallData(const GrpcAsyncCallData&) = delete; + GrpcAsyncCallData& operator=(const GrpcAsyncCallData&) = delete; + GrpcAsyncCallData(GrpcAsyncCallData&&) = delete; + GrpcAsyncCallData& operator=(GrpcAsyncCallData&&) = delete;
110-115: Consider propagating the result ofresult_callback
result_callbackreturns abool, yet the lambda ignores it.
If callers use that return value to signal retry/cleanup decisions, the information is silently lost here.
At minimum, document that the return value is ignored, or forward it to the caller by storing it incall_data->grpc_status/returning a different status code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/build-tarball.yml(2 hunks).github/workflows/coverage-linux-without-intl.yml(1 hunks).github/workflows/coverage-linux.yml(1 hunks).github/workflows/test-linux.yml(1 hunks).github/workflows/test-macos.yml(1 hunks)agents/grpc/src/grpc_agent.cc(10 hunks)agents/grpc/src/grpc_client.cc(0 hunks)agents/grpc/src/grpc_client.h(2 hunks)
💤 Files with no reviewable changes (1)
- agents/grpc/src/grpc_client.cc
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/coverage-linux.yml
- .github/workflows/build-tarball.yml
- .github/workflows/test-linux.yml
- .github/workflows/test-macos.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/coverage-linux-without-intl.yml
- agents/grpc/src/grpc_agent.cc
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-linux-without-intl
- GitHub Check: build-tarball
- GitHub Check: test-macOS
- GitHub Check: coverage-linux
- GitHub Check: coverage-windows
- GitHub Check: test-linux
santigimeno
changed the base branch from
node-v22.x-nsolid-v5.x
to
santi/merge_22_15_0
santigimeno
force-pushed
the
santi/dry_grpc_client
branch
from
b897424 to
c3bc0ad
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
agents/grpc/src/grpc_client.h (1)
98-109: Replace ASSERT_NOT_NULL with runtime checks.While ASSERT_NOT_NULL helps in debug builds, these assertions might be removed in release builds, potentially leading to crashes if null pointers are passed. Consider adding explicit null checks that return error codes in all build configurations.
- ASSERT_NOT_NULL(stub); - ASSERT_NOT_NULL(exportFunc); + if (stub == nullptr || exportFunc == nullptr) { + return -1; + }
🧹 Nitpick comments (1)
agents/grpc/src/grpc_client.h (1)
83-97: Parameter type could be simplified.The parameter
EventT&& eventuses a forwarding reference, but the implementation simply moves the value. Since perfect forwarding isn't needed here, consider simplifying toEventT eventto improve readability and maintain the same behavior.- EventT&& event, + EventT event,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
agents/grpc/src/grpc_agent.cc(10 hunks)agents/grpc/src/grpc_client.cc(0 hunks)agents/grpc/src/grpc_client.h(3 hunks)
💤 Files with no reviewable changes (1)
- agents/grpc/src/grpc_client.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- agents/grpc/src/grpc_agent.cc
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-tarball-linux
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-macOS
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-windows
- GitHub Check: coverage-linux
- GitHub Check: coverage-linux-without-intl
🔇 Additional comments (5)
agents/grpc/src/grpc_client.h (5)
25-50: The template class is well-designed.Moving the
GrpcAsyncCallDatatemplate class from the implementation file to the header is appropriate for template code. The class properly manages resources, disables copying/moving, and clearly defines the lifecycle of objects.
103-106: Good use of Arena allocation.Using
Arena::Createfor both event and response objects ensures they have the proper lifetime and addresses the previous dangling pointer issue. The arena is correctly passed to the callback, ensuring it remains valid until the callback completes.
72-82: Clear and helpful documentation.The documentation with usage examples greatly improves the usability of this templated method, making it clear how to call it with different event types.
107-109: Proper allocation failure handling.Returning -1 instead of using assert(0) is a better approach for handling allocation failures, as it allows callers to handle the error gracefully even in release builds.
114-123: Template method invocation is clear and correct.The code properly uses the template function pointer to call the correct export method on the stub. The lambda capture and callback forwarding also look correct, with proper resource management.
santigimeno
force-pushed
the
santi/merge_22_15_0
branch
2 times, most recently
from
4fbc831 to
e76de7a
Compare
santigimeno
force-pushed
the
santi/dry_grpc_client
branch
from
c3bc0ad to
6b2db83
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
agents/grpc/src/grpc_client.h (1)
84-97: Parameter type for event could be improved.The function takes
EventT&& eventbut later moves from it. It would be clearer to use either:
EventT event(by value) - explicitly showing the intent to take ownershipconst EventT& event+ copy in the implementation - if copying is preferredThe current approach works but could lead to confusion about ownership transfer.
- EventT&& event, + EventT event,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
agents/grpc/src/grpc_agent.cc(10 hunks)agents/grpc/src/grpc_client.cc(0 hunks)agents/grpc/src/grpc_client.h(3 hunks)
💤 Files with no reviewable changes (1)
- agents/grpc/src/grpc_client.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- agents/grpc/src/grpc_agent.cc
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: lint-py
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: lint-cpp
- GitHub Check: lint-js-and-md
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: build-tarball
- GitHub Check: build-docs
- GitHub Check: coverage-windows
- GitHub Check: coverage-linux
- GitHub Check: coverage-linux-without-intl
- GitHub Check: test-macOS
🔇 Additional comments (6)
agents/grpc/src/grpc_client.h (6)
4-4: Good addition of the assertions header.The inclusion of "asserts-cpp/asserts.h" is appropriate for the ASSERT_NOT_NULL macros used later in the code.
18-18: LGTM: Clean addition of using directive.The using directive for
google::protobuf::Arenareduces verbosity in the code that follows.
25-50: Well-designed async call data template class.Moving the
GrpcAsyncCallDataclass from the source file to the header is appropriate for template visibility. The class is well-structured with:
- Clear member variable organization
- Proper memory ownership semantics with unique_ptr
- Complete rule-of-five implementation (disabling copy/move operations)
- Default constructor and destructor
This implementation properly encapsulates all the state needed for an asynchronous gRPC call.
72-82: Excellent documentation with usage example.The function documentation clearly explains how to use the templated method and provides a concrete usage example, which is very helpful for maintainers.
98-109: Good null checks and error handling.The implementation properly:
- Asserts that both
stubandexportFuncare not null- Creates and initializes the call data
- Allocates the event and response in the Arena
- Returns an error code when allocation fails
This addresses previous review feedback about proper error handling.
111-126: Clean implementation of async call delegation.The implementation:
- Properly transfers ownership of objects
- Correctly captures shared_ptr to call_data in the lambda
- Sets up appropriate callback chaining
- No longer deletes Arena-allocated memory
This addresses previous concerns about object lifetime and memory management.
This commit refactors the GrpcClient::DelegateAsyncExport method to use templates instead of having multiple overloaded implementations for each event type. This change significantly reduces code duplication while maintaining the same functionality, making the codebase more maintainable and less prone to errors when adding new event types.
santigimeno
force-pushed
the
santi/dry_grpc_client
branch
from
6b2db83 to
43911c9
Compare
This commit refactors the GrpcClient::DelegateAsyncExport method to use templates instead of having multiple overloaded implementations for each event type. This change significantly reduces code duplication while maintaining the same functionality, making the codebase more maintainable and less prone to errors when adding new event types. PR-URL: #294 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
|
Landed in d0c432e |
This commit refactors the GrpcClient::DelegateAsyncExport method to use templates instead of having multiple overloaded implementations for each event type. This change significantly reduces code duplication while maintaining the same functionality, making the codebase more maintainable and less prone to errors when adding new event types. PR-URL: #294 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
This commit refactors the GrpcClient::DelegateAsyncExport method to use templates instead of having multiple overloaded implementations for each event type. This change significantly reduces code duplication while maintaining the same functionality, making the codebase more maintainable and less prone to errors when adding new event types. PR-URL: #294 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
This commit refactors the GrpcClient::DelegateAsyncExport method to use templates instead of having multiple overloaded implementations for each event type. This change significantly reduces code duplication while maintaining the same functionality, making the codebase more maintainable and less prone to errors when adding new event types. PR-URL: #294 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
This commit refactors the GrpcClient::DelegateAsyncExport method to use templates instead of having multiple overloaded implementations for each event type. This change significantly reduces code duplication while maintaining the same functionality, making the codebase more maintainable and less prone to errors when adding new event types. PR-URL: #294 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
2 participants
This commit refactors the GrpcClient::DelegateAsyncExport method to use templates instead of having multiple overloaded implementations for each event type.
This change significantly reduces code duplication while maintaining the same functionality, making the codebase more maintainable and less prone to errors when adding new event types.
Summary by CodeRabbit