Re: [PR] CAMEL-20794: AWS2 Kinesis producer supports async client [camel]

[via GitHub]

davsclaus commented on code in PR #14238:
URL: https://github.com/apache/camel/pull/14238#discussion_r1614378502


##
components/camel-aws/camel-aws2-kinesis/src/main/java/org/apache/camel/component/aws2/kinesis/Kinesis2Producer.java:
##
@@ -47,11 +53,28 @@ public Kinesis2Endpoint getEndpoint() {
 
 @Override
 public void process(Exchange exchange) throws Exception {
+Exception exceptionThrownByPutRecord = this.exceptionThrownByPutRecord;
+if (exceptionThrownByPutRecord != null) {
+this.exceptionThrownByPutRecord = null;
+throw exceptionThrownByPutRecord;
+}
+
 PutRecordRequest request = createRequest(exchange);
-PutRecordResponse putRecordResult = 
connection.getClient(getEndpoint()).putRecord(request);
-Message message = getMessageForResponse(exchange);
-message.setHeader(Kinesis2Constants.SEQUENCE_NUMBER, 
putRecordResult.sequenceNumber());
-message.setHeader(Kinesis2Constants.SHARD_ID, 
putRecordResult.shardId());
+if (getEndpoint().getConfiguration().isAsyncClient()) {

Review Comment:
   This is wrong, as you cannot use async client in a camel producer that is 
fully synchronous. Camel needs to be aware that this is async and route 
accordingly - this kind of work is complex and takes time to understand how to 
do it.



##
components/camel-aws/camel-aws2-kinesis/src/main/java/org/apache/camel/component/aws2/kinesis/Kinesis2Producer.java:
##
@@ -47,11 +53,28 @@ public Kinesis2Endpoint getEndpoint() {
 
 @Override
 public void process(Exchange exchange) throws Exception {
+Exception exceptionThrownByPutRecord = this.exceptionThrownByPutRecord;

Review Comment:
   This is not as good as concurrent calls could lead to thread safety issues - 
Use AtomicInstance or what the name is.
   
   Also its not understandable why a previous exception should cause the next 
to fail. There is no batch stuff here. 
   
   I think you should seperate the items you try to help with
   
   - batching (make this work with current client)
   - async (much harder - wait with this as its not needed asap)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] CAMEL-20794: AWS2 Kinesis producer supports async client [camel]

[via GitHub]

github-actions[bot] commented on PR #14238:
URL: https://github.com/apache/camel/pull/14238#issuecomment-2130021827

   :star2: Thank you for your contribution to the Apache Camel project! :star2: 
   
   :robot: CI automation will test this PR automatically.
   
   :camel: Apache Camel Committers, please review the following items:
   
   * First-time contributors **require MANUAL approval** for the GitHub Actions 
to run
   
   * You can use the command `/component-test (camel-)component-name1 
(camel-)component-name2..` to request a test from the test bot.
   
   * You can label PRs using `build-all`, `build-dependents`, `skip-tests` and 
`test-dependents` to fine-tune the checks executed by this PR.
   
   * Build and test logs are available in the Summary page. **Only** [Apache 
Camel committers](https://camel.apache.org/community/team/#committers) have 
access to the summary. 
   
   * :warning: Be careful when sharing logs. Review their contents before 
sharing them publicly.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org