Croway commented on PR #29269:
URL: https://github.com/apache/camel/pull/29269#issuecomment-6669780684
@davsclaus thanks for the review. I checked the four points on the PR head,
and all of them hold. They are addressed in 3 new commits:
- **Ordering:** confirmed. With A failing once and B (same partition key)
written, the stream order is `[B, A]`. The Batch Producer section now says that
a record that is sent again is written after the next records of its request.
It also says that `PutRecords` does not guarantee the order anyway (the AWS
PutRecords reference says so too), and it points to single-record sends (a body
that is not an `Iterable`) when the order of a partition key matters. The 4.24
upgrade-guide entry mentions it too. 13e77dd52a6f
- **When `putRecords` itself throws:** confirmed. On the PR head, after a
call that throws, both headers were `null`. I set the counts in that path too,
because the upgrade guide already said that the headers are set when the
exchange fails, and that is now true for both failure paths. The headers are
set in a `finally`, so the exception of the call still reaches the caller as it
is, and the next requests are still not sent. In that case:
- `CamelAwsKinesisRecordCount` is the number of records of the batch, as
on the other paths;
- `CamelAwsKinesisFailedRecordCount` counts every record that Kinesis did
not report as written: the records of the call that threw (even though, after a
timeout, some of them may be written), those of the requests that were not
sent, and those of earlier requests that still failed after the retries. So
"count - failed" is the number of records that Kinesis confirmed as written,
including the records of the same request that were written before a retry
threw.
New test `batchSetsTheCountsWhenAPutRecordsCallThrows`: 1200 records, r600
of the 2nd request fails, and the call that sends it again throws a
`KmsThrottlingException`. The same exception instance is thrown (`assertSame`),
the calls are `[500, 500, 1]`, 999 records are written, and the headers are
`1200` / `201`. It fails on the PR head with `expected: <1200> but was:
<null>`. The component doc and the upgrade-guide entry are updated. d5d423e2a3c7
- **Worst case:** confirmed, with the numbers. A request sleeps 3 times
(before retries 1 to 3): 100 + 200 + 400 = 700 ms, and the random part adds up
to 25 + 50 + 100 = 175 ms, so 700 to 875 ms per request. A batch of 10,000
records is 20 requests of 500, so 14 to 17.5 s plus the 80 calls and the SDK
retries (more requests, and a longer wait, when the records are large). With a
mocked client that throttles every record I measured 80 calls and 16.0 s. That
line is now in the Batch Producer section and in the upgrade-guide entry.
13e77dd52a6f
- **Nit (short response):** fixed. A record without a result is now counted
as failed, so it is sent again like a failed record. If it still has no result
after the last retry, it is counted in `CamelAwsKinesisFailedRecordCount`, and
the exception says `MissingResult: The PutRecords response has no result for
this record`. New test `batchSendsAgainARecordWithoutResultInTheResponse` stubs
a response with 2 results for 3 records: the calls were `[3]` before the fix
and are `[3, 1]` now, and all 3 records are written. d7fcde5c460f
`Kinesis2ProducerTest` has 14 tests and they all pass. The module suite
passes: 64 unit tests.
_Claude Code on behalf of Federico Mariani_
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
