CAMEL-23952: Support executeToolsConcurrently for langchain4j-agent - #25228
CAMEL-23952: Support executeToolsConcurrently for langchain4j-agent#25228atiaomar1978-hub wants to merge 6 commits into
Conversation
|
AI-generated comment on behalf of atiaomar1978-hub Follow-up commit
Re-ran |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
AI-generated comment on behalf of atiaomar1978-hub Pushed The build regenerated catalog docs from Please re-run or wait for CI on the latest commit. |
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 tested, 28 compile-only — current: 10 all testedMaveniverse Scalpel detected 39 affected modules (current approach: 10).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Well-structured PR. The core design is sound: duplicate() protects shared registry beans from mutation, Camel's ExecutorServiceManager manages the thread pool lifecycle correctly (doInit create, doStop shutdown), and the concurrency test genuinely validates parallel tool overlap using a CountDownLatch barrier + AtomicInteger high-water-mark. CI is green. No correctness issues — only convention cleanups below.
Findings
1. [Convention] Test methods use public and JUnit assertions instead of AssertJ — AgentConfigurationTest.java
The two new test methods (testDuplicateCopiesExecuteToolsConcurrentlySettings, testExecuteToolsConcurrently) use public visibility and JUnit assertions (assertTrue, assertSame, assertEquals, assertNull). Per project conventions: new test methods should be package-private and use AssertJ where it improves readability. Example migration:
@Test
void testExecuteToolsConcurrently() {
AgentConfiguration config = new AgentConfiguration();
assertThat(config.getExecuteToolsConcurrently()).isNull();
assertThat(config.getExecuteToolsExecutor()).isNull();
AgentConfiguration enabled = config.withExecuteToolsConcurrently();
assertThat(enabled).isSameAs(config);
assertThat(config.getExecuteToolsConcurrently()).isTrue();
// ...
}2. [Convention] FQCNs in test code — LangChain4jAgentExecuteToolsConcurrentlyTest.java
Two method signatures use fully-qualified class names instead of imports:
bindToRegistry(org.apache.camel.spi.Registry registry)→ importRegistryrecordConcurrentEntry(String label, org.apache.camel.Exchange exchange)→ importExchange
3. [Nit] assertSame on autoboxed int is fragile — AgentConfigurationTest.java
assertSame(original.getMaxToolCallingRoundTrips(), copy.getMaxToolCallingRoundTrips());getMaxToolCallingRoundTrips() returns int, so both sides are autoboxed. For value 3 this works due to Integer caching (-128 to 127), but for values > 127 it would fail. The preceding assertEquals already validates the value — this line can be removed or changed to assertEquals/assertThat(...).isEqualTo(...).
4. [Suggestion] Tests leak Executors.newSingleThreadExecutor() instances — AgentConfigurationTest.java
Both new test methods create single-thread executors without shutdown, leaking non-daemon threads. Minor — won't cause test failures — but wrapping in try/finally with executor.shutdownNow() would be cleaner.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Wire LangChain4j parallel tool execution through AgentConfiguration, resolve a Camel-managed executor when none is supplied, and add tests plus component documentation. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid mutating registry-held configuration when attaching the Camel-managed concurrent tool pool; add duplicate() and a context restart regression test. Co-authored-by: Cursor <cursoragent@cursor.com>
Regenerate catalog copy of component adoc so sourcecheck passes after executeToolsConcurrently documentation was added. Co-authored-by: Cursor <cursoragent@cursor.com>
Migrate new AgentConfigurationTest methods to AssertJ, shut down test executors, remove fragile assertSame on autoboxed int, and replace FQCNs with imports in LangChain4jAgentExecuteToolsConcurrentlyTest. Co-authored-by: Cursor <cursoragent@cursor.com>
a010283 to
3a5d72c
Compare
|
AI-generated comment on behalf of atiaomar1978-hub Addressed @gnodet review in
Re-ran Ready for re-review. |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Review Summary
Well-structured PR that adds concurrent tool execution support for langchain4j-agent. The core design is sound: duplicate() protects shared registry beans from mutation, Camel's ExecutorServiceManager manages the thread pool lifecycle correctly, and the concurrency test genuinely validates parallel tool overlap.
All findings from the previous review have been addressed: test methods now use package-private visibility, AssertJ assertions, isEqualTo instead of assertSame on primitives, and proper try/finally with shutdownNow() for executor cleanup.
The concurrency test (LangChain4jAgentExecuteToolsConcurrentlyTest) is particularly well-designed with CountDownLatch(2) barrier to prove both tools are executing simultaneously. The documentation addition correctly notes that tool-route header side-effects are not merged back onto the main exchange when tools run concurrently.
Minor suggestions (non-blocking)
duplicate()fragility: Consider adding a reflection-based test that verifies all declared instance fields are copied byduplicate()— prevents copy staleness when new fields are added toAgentConfiguration.- Null guard on
withExecuteToolsConcurrently(Executor): Passingnullsilently enables the managed executor fallback. A defensiveObjects.requireNonNull()would make the API self-documenting sincewithExecuteToolsConcurrently()(no-arg) already exists for that intent. - Test class visibility:
AgentConfigurationTestclass declaration is stillpublic— per project convention it should be package-private.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
… API Reject null executors in withExecuteToolsConcurrently(Executor), add a reflection test ensuring duplicate() copies every instance field, and make AgentConfigurationTest package-private. Co-authored-by: Cursor <cursoragent@cursor.com>
|
AI-generated comment on behalf of atiaomar1978-hub Pushed
Tests (all green locally):
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-review: All suggestions addressed ✅
Commit a9f0680 cleanly addresses all three suggestions from the previous review:
-
duplicate()fragility — AddedduplicateCopiesAllDeclaredInstanceFields()using reflection to verify every non-static instance field is copied. Usesfield.setAccessible(true)and AssertJ.as()messages for clear failure diagnostics. -
Null guard on
withExecuteToolsConcurrently(Executor)— Now rejects null withObjects.requireNonNull, backed bywithExecuteToolsConcurrentlyRejectsNullExecutortest usingassertThatThrownBy. Javadoc enhanced with@throws NullPointerExceptionand cross-reference to the no-arg variant. -
Test class visibility —
AgentConfigurationTestclass declaration is now package-private.
New test methods consistently follow project conventions: package-private visibility, AssertJ assertions, proper executor cleanup via try/finally.
Minor non-blocking note
The reflection test doesn't populate all fields on the original (e.g., chatModel, outputGuardrailClasses), so some equality checks trivially pass with null==null. Populating a few more fields would strengthen coverage, but this is a very minor point.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
|
I think @Croway also did something like this or what was it that he did ? |
|
AI-generated comment on behalf of atiaomar1978-hub @davsclaus Good question — I looked through Croway's langchain4j-related work and there is no prior PR that exposed LangChain4j's What Croway did do in the same area (and what this PR builds on):
This PR adds the LangChain4j-side parallel dispatch ( So if you had MCP integration in mind — yes, that's Croway's piece. The concurrent execution toggle itself is new here. |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Clean, well-tested implementation. The duplicate() pattern correctly prevents mutation of registry-held configuration beans, and the reflection-based test is a nice maintenance safeguard against future field additions. Managed executor lifecycle follows standard Camel ExecutorServiceManager conventions.
One observation (non-blocking): the managed executor is created in doInit() and shut down in doStop(). A route-level stop/start (without context restart) won't recreate it — but this matches the existing pattern where the agent field is also only created in doInit().
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Populate chatModel, output guardrails, and customTools in the reflection-based duplicate() test so field equality checks exercise non-null values per review feedback. Co-authored-by: Cursor <cursoragent@cursor.com>
|
AI-generated comment on behalf of atiaomar1978-hub Follow-up (
|
3 participants
Summary
AI-generated on behalf of atiaomar1978-hub
AgentConfiguration.withExecuteToolsConcurrently()andwithExecuteToolsConcurrently(Executor), wired intoAbstractAgent.configureBuilder()for LangChain4jAiServices.LangChain4jAgentProducerregisters a Camel-managed thread pool viaExecutorServiceManager(on a duplicated configuration so registry beans are not mutated) and shuts it down on component stop.Review follow-up (Bugbot / Grok)
#agentConfigurationregistry beans; producer usesAgentConfiguration.duplicate()for the build-time copy.duplicate()unit test.Test plan
AgentConfigurationTest(executeToolsConcurrently + duplicate)LangChain4jAgentExecuteToolsConcurrentlyTest(parallel overlap, registry not mutated, restart)mvn test -pl components/camel-ai/camel-langchain4j-agent -am -Dtest=LangChain4jAgentExecuteToolsConcurrentlyTest,AgentConfigurationTest