CAMEL-23295: Fix resource leak and improve error handling in camel-splunk-hec producer#22483
CAMEL-23295: Fix resource leak and improve error handling in camel-splunk-hec producer#22483
Conversation
…lunk-hec producer - Add warning log when skipTlsVerify is enabled to alert operators - Replace RuntimeException with RuntimeCamelException for proper Camel error handling - Log error response body at DEBUG level for easier debugging - Consume response entity on success path to prevent potential connection leaks - Add unit tests for error handling behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Review
Claude Code on behalf of Guillaume Nodet
Overall a solid, well-scoped bug fix. A couple of items worth addressing:
Issues
1. (Medium) Missing CamelContext cleanup in test
SplunkHECProducerTest.java — @BeforeEach creates and starts a DefaultCamelContext but there is no @AfterEach to stop/close it. This leaks resources across test runs. Please add:
@AfterEach
void tearDown() {
camelContext.close();
}2. (Low) Third test is redundant
testExceptionTypeIsRuntimeCamelExceptionNotRuntimeException catches Exception.class and asserts the class is exactly RuntimeCamelException.class. The other two tests already use assertThrows(RuntimeCamelException.class, ...) — if the type were wrong, those would fail first. Consider removing this test to keep the test class focused.
Positives
RuntimeException→RuntimeCamelExceptionfollows Camel conventionsEntityUtils.consume()on the success path properly prevents connection pool leaks- TLS verification warning is appropriate
- Good test coverage for the error paths
This review covers project conventions, correctness, and test coverage. It does not replace specialized tools like SonarCloud or CodeRabbit.
|
🧪 CI tested the following changed modules:
All tested modules (7 modules)
|
- Add @AfterEach to close CamelContext and prevent resource leak in tests - Remove redundant testExceptionTypeIsRuntimeCamelExceptionNotRuntimeException test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
Claude Code on behalf of Andrea Cosentino @gnodet Thank you for the review! Both items addressed in 3c5e848:
|
Summary
Claude Code on behalf of Andrea Cosentino
skipTlsVerifyis enabled to alert operators that TLS verification is disabledRuntimeExceptionwithRuntimeCamelExceptionfor proper Camel error handlingFixes: https://issues.apache.org/jira/browse/CAMEL-23295
Test plan
SplunkHECProducerTestverifiesRuntimeCamelExceptionis thrown on 400 and 503 responses@AfterEachproperly closes CamelContext to prevent resource leaks in tests🤖 Generated with Claude Code