CAMEL-23338: added OpenSearchClient as parameter to pass custom instance#24320
CAMEL-23338: added OpenSearchClient as parameter to pass custom instance#24320renjth-81 wants to merge 1 commit into
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
you need to also rebuild camel-catalog, and some code in component and endpoint dsl |
|
There are uncommitted changes |
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @renjth-81 — being able to pass a custom OpenSearchClient is a useful addition, and the generated catalog/DSL files are all correctly regenerated. However, there's a blocking correctness problem in the current implementation that CI is also catching, so I'm requesting changes.
Blocker: the cached transport captures a null RestClient → NPE on every exchange
OpensearchProducer now builds and caches openSearchClient in its constructor:
- the constructor sets
this.client = endpoint.getClient();thenthis.openSearchClient = createDefaultOpenSearchClient(); createDefaultOpenSearchClient()buildsnew OpenSearchClient(new RestClientTransport(client, ...)), but only callsstartClient()whenconfiguration.isDisconnect() && client == null.
In the standard configuration (a RestClient built from hostAddresses, no autowired client), endpoint.getClient() is null at construction time — the component creates the endpoint with a null client by default (OpensearchComponent.java:91), and the real RestClient is created lazily later in doStart() → startClient() (OpensearchProducer.java, the double-checked-locking block around lines 452–467). disconnect defaults to false (OpensearchConfiguration.java:60), so createDefaultOpenSearchClient() does not start the client and ends up wrapping a RestClientTransport around a null RestClient.
process() then does OpenSearchTransport transport = openSearchClient._transport(); and reuses that cached transport, so every exchange dereferences the null RestClient:
java.lang.NullPointerException: Cannot invoke
"org.opensearch.client.RestClient.performRequestAsync(...)" because "this.restClient" is null
This is exactly what CI is failing on — build (17, false) and build (25, false) are both red, with OpensearchPingIT.testPing and all OpensearchBulkIT.* failing with this NPE. The user-supplied-client path (endpoint.getOpenSearchClient() != null) works, but the default path — which all of the component's own integration tests exercise — is broken.
Suggested direction
Resolve the transport lazily rather than in the constructor: keep using the user-supplied OpenSearchClient when one is provided, but for the default case build the transport in process() / after doStart() once client is non-null (as the original code did). Please also keep the disconnect=true semantics in mind — the old process() rebuilt the transport per call and cleanup() closes and nulls the client after each exchange (OpensearchProducer.java, around lines 437–445), so a single cached transport would also break disconnect mode after the first exchange.
Other items
- Tests: please add a test proving a custom
OpenSearchClientinstance is actually honored at runtime — the new option currently has no coverage, and the existing integration tests regressed. - Jira: apache/camel changes should be tracked by a
CAMEL-XXXXXissue with the commit/PR title prefixed accordingly. Could you create one (componentcamel-opensearch) and update the title? Theadded missing filescommit subject could also be made more descriptive.
For context: this PR currently shows as approved, but that review was on the very first commit (62b5af1), before the generated-files commit and before CI ran — so it predates the failures above.
Thanks again for working on this — happy to re-review once the transport init is made lazy and CI is green.
Reviewed with Claude Code on behalf of Andrea Cosentino (@oscerd). This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
creating a new client should ideally not be done in constructors but in doInit or doStart |
renjth-81
changed the title
renjth-81
changed the title
|
@davsclaus @oscerd I have pushed the fixes and integration test. Please could you review |
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 12 tested, 26 compile-only — current: 38 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 38). Skip-tests mode would test 12 modules (4 direct + 8 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (12)
Modules with tests skipped (26)
All tested modules (38 modules)
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for addressing the NPE blocker from the previous review, @renjth-81 — the lazy transport resolution in process() is the right approach. However, there are a couple of remaining issues that need fixing before this can be merged.
Blocker: Missing Mockito dependency
The new test CustomOpensearchClientPingIT.java uses Mockito.mock(), Mockito.when(), and RETURNS_SMART_NULLS, but components/camel-opensearch/pom.xml does not declare mockito-core as a test dependency. No transitive path brings it in either (camel-test-junit6, camel-test-infra-core, and camel-test-infra-opensearch do not depend on Mockito). The module will fail to compile.
Please add to pom.xml:
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>${mockito-version}</version>
<scope>test</scope>
</dependency>Bug: Transport closed on user-provided client when disconnect=true
In OpensearchProducer.cleanup(), IOHelper.close(ctx.transport()) is called when disconnect=true. For the default path this is safe — a fresh RestClientTransport is created per process() call. But when a custom OpenSearchClient is provided, ctx.transport() is openSearchClient._transport() — the user's own transport. Closing it will corrupt the user's client for future use.
The test exercises this path (disconnect=true + custom client), but since the mock's close() is a no-op, the bug is masked.
Suggested fix: skip closing the transport and client in cleanup() when a custom OpenSearchClient was provided.
Minor: doStart() logs spurious warning with custom client
doStart() unconditionally calls startClient() when disconnect=false. When a user provides only an OpenSearchClient (no hostAddresses), startClient() logs: "Incorrect ip address and port parameters settings for OpenSearch cluster". Consider skipping startClient() when a custom OpenSearchClient is set.
Minor: Commit hygiene
- Per project conventions, commits should be prefixed with
CAMEL-23338:. - There's a merge commit and duplicate commit messages — please squash/clean up before merge.
- A spurious blank line was added in
OpensearchProducer.javabetweenInteger from = message.getHeader(...)andif (from == null)— please remove.
Note: this review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analysis (SonarCloud).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
|
||
| import org.apache.camel.builder.RouteBuilder; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.Mockito; |
There was a problem hiding this comment.
Missing dependency: mockito-core is not declared as a test dependency in components/camel-opensearch/pom.xml and is not transitively available. This import (and all Mockito usage in this file) will cause a compilation failure.
Please add to pom.xml:
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>${mockito-version}</version>
<scope>test</scope>
</dependency>| final ObjectMapper mapper = new ObjectMapper(); | ||
| mapper.configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false); | ||
| OpenSearchTransport transport = new RestClientTransport(client, new JacksonJsonpMapper(mapper)); | ||
| // 2. Index and type will be set by: |
There was a problem hiding this comment.
Transport lifecycle bug with disconnect=true: When a custom OpenSearchClient is provided and disconnect=true, cleanup() (line ~438) calls IOHelper.close(ctx.transport()). In this branch, transport is openSearchClient._transport() — the user's own transport. Closing it will corrupt the user's client.
The default path creates a fresh RestClientTransport per call, so closing it is safe. But the custom client's transport should not be closed by the component.
Suggested fix: pass a flag into ActionContext indicating whether the transport is user-owned, and skip IOHelper.close(ctx.transport()) in cleanup() when it is.
| Integer size = message.getHeader(OpensearchConstants.PARAM_SIZE, Integer.class); | ||
| if (size == null) { | ||
| message.setHeader(OpensearchConstants.PARAM_SIZE, configuration.getSize()); | ||
| } |
There was a problem hiding this comment.
Nit: this blank line was not present before and is not needed — please remove to keep the diff clean.
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Re-review after the latest push — good progress, the Round 1 items are addressed:
- The transport is now resolved lazily in
process()(not in the constructor), so the default path no longer NPEs, andOpensearchPingIT/BulkITpass. CustomOpensearchClientPingITproves a customOpenSearchClientis actually used.- The title now carries the
CAMEL-23338:prefix.
Remaining items (matching @davsclaus's review):
disconnect=true+ a custom client closes the caller's transport. WhenopenSearchClientis provided,process()setstransport = openSearchClient._transport()and passes it into theActionContext;cleanup()then doesIOHelper.close(ctx.transport())wheneverconfiguration.isDisconnect()is true (OpensearchProducer.java:437-438). So withdisconnect=trueand a user-supplied client, the component closes the caller's transport after the first exchange, corrupting their client. The custom-client path should not close a transport it does not own.- Commit hygiene: the individual commits are not
CAMEL-23338:-prefixed and there is a merge commit + duplicate messages — a squash at merge would tidy this up.
On the "missing Mockito dependency" point: mockito-core is pulled in transitively via camel-test-junit6, which is why the module compiles and CI is green — an explicit declaration is optional rather than required.
Thanks for iterating on this!
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
renjth-81
force-pushed
the
opensearch-custom-openSearchClient
branch
from
7f12e7f to
d0a838a
Compare
|
@davsclaus @oscerd Please could you review the changes |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Claude Code on behalf of davsclaus
Review Summary
Thank you for this contribution — the ability to inject a custom OpenSearchClient is a valuable addition, especially for AWS OpenSearch Service users who need custom transport options. The overall approach is sound. A few items need attention before this can be merged.
Note: This review covers project rules and conventions. It does not replace specialized review tools such as CodeRabbit, Sourcery, or SonarCloud.
Documentation
The PR adds a new user-visible endpoint option (openSearchClient) but does not include a documentation update. Per project guidelines, any user-visible change must be documented. Please update the opensearch component docs page to describe the new option and when to use it (e.g., AWS OpenSearch Service with custom transport/credentials).
Precedence Behavior
When both client (RestClient) and openSearchClient (OpenSearchClient) are configured — possible via autowiring since both are marked autowired = true — the openSearchClient silently takes precedence. Consider documenting this precedence in the option description, or logging a warning when both are set.
Test Coverage
The integration test only covers the Ping operation with disconnect=true/false. Other operations (Index, Search, Bulk, etc.) are not tested with the custom client path. Consider adding at least one test for an operation that exercises the data path (e.g., Index or Search).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| this.client = endpoint.getClient(); | ||
| this.openSearchClient = endpoint.getOpenSearchClient(); | ||
| if(this.openSearchClient != null) { | ||
| isCustomClient = true; |
There was a problem hiding this comment.
The isCustomClient field is always equivalent to openSearchClient != null — both are set once in the constructor and never modified. This field adds unnecessary complexity.
The if/else block also has formatting issues (missing space after if, missing spaces around else).
Suggestion: remove the isCustomClient field entirely and use openSearchClient != null directly at the 3 call sites.
| isCustomClient = true; | |
| this.openSearchClient = endpoint.getOpenSearchClient(); |
| startClient(); | ||
| OpenSearchTransport transport; | ||
| if (openSearchClient == null) { | ||
| if (configuration.isDisconnect() && client == null && !isCustomClient) { |
There was a problem hiding this comment.
Inside this if (openSearchClient == null) branch, the && !isCustomClient check is always true (since isCustomClient is only true when openSearchClient != null). This is redundant and can be simplified to:
| if (configuration.isDisconnect() && client == null && !isCustomClient) { | |
| if (configuration.isDisconnect() && client == null) { |
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-core</artifactId> | ||
| <version>${mockito-version}</version> |
There was a problem hiding this comment.
Minor: the child elements use 2-space indentation here, but the rest of the POM uses 4-space indentation for dependency children. Running mvn formatter:format should fix this automatically.
| <version>${mockito-version}</version> | |
| <groupId>org.mockito</groupId> | |
| <artifactId>mockito-core</artifactId> | |
| <version>${mockito-version}</version> | |
| <scope>test</scope> |
Actually, please just run mvn formatter:format on this module to align with the project style.
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Re-reviewing after your squash + latest changes — the items from my earlier note are all resolved: the disconnect=true + custom-client case now guards the close (!isCustomClient), the spurious "incorrect ip/port" warning is skipped for a custom client, and the history is squashed into a single CAMEL-23338: commit. Nice.
The one thing that will still turn CI red is the formatting gate — if(this.openSearchClient != null) / }else{ need spaces (mvn formatter:format impsort:sort fixes it). That, plus the rest of @davsclaus's latest list, are the remaining items. Thanks for iterating on this!
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of @gnodet
Review: CAMEL-23338 — Add OpenSearchClient as parameter
The core approach is sound — lazy transport resolution in process(), skipping client lifecycle for custom client. However, several items need attention before merging, and prior review feedback from @davsclaus still needs to be addressed:
1. BLOCKER: Formatting violations fail CI
In OpensearchProducer.java, the constructor has if(this.openSearchClient != null) and }else{ — missing spaces after if and around else. Please run:
mvn formatter:format impsort:sort2. BLOCKER: Redundant isCustomClient field
As @davsclaus already noted, the isCustomClient boolean field is always equivalent to openSearchClient != null — both are set once in the constructor and never mutated. This adds unnecessary state. On line 167, the check if (configuration.isDisconnect() && client == null && !isCustomClient) is inside a branch that already established openSearchClient == null, making !isCustomClient always true — dead code.
Please remove the field and use openSearchClient != null directly.
3. Missing component documentation update
No .adoc files were modified. The new openSearchClient endpoint option should be documented in components/camel-opensearch/src/main/docs/opensearch-component.adoc with its use case (e.g., AWS OpenSearch Service with custom transport/credentials).
4. POM indentation mismatch
The added mockito-core dependency in pom.xml uses 2-space indentation while the rest of the POM uses 4-space. Also, mockito-core is already available transitively via camel-test-junit6 — the explicit dependency may be redundant.
5. Precedence ambiguity when both clients are set
Both RestClient client and OpenSearchClient openSearchClient are autowired = true. If a user's registry contains both, openSearchClient silently takes precedence. This should be documented in the @UriParam description (e.g., "When set, this takes precedence over the RestClient-based client option").
Thanks for this contribution — the feature is useful and the approach is correct. Please address the formatting, the isCustomClient removal, and the documentation, and this should be ready.
4 participants
Description
Added OpenSearchClient as a parameter to allow custom OpenSearchClient instance to be passed.
Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.