CAMEL-23952: Support executeToolsConcurrently for langchain4j-agent - #25228
CAMEL-23952: Support executeToolsConcurrently for langchain4j-agent#25228atiaomar1978-hub wants to merge 4 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. |
2 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