test: fix unstable batch consumer in MessageDrivenAdapterTests.#10942
Conversation
By starting the adapter after sending messages, ensure a single poll retrieves the full batch messages. Signed-off-by: Jiandong Ma <jiandong.ma.cn@gmail.com>
| template.sendDefault(0, 1487694048608L, 1, "bar"); | ||
|
|
||
| adapter.start(); | ||
| ContainerTestUtils.waitForAssignment(container, 1); |
There was a problem hiding this comment.
Thanks for looking into this @mjd507 !
Well, I don't think it helps to make sure both records are in the topic because those max.record and so Consumer properties should do the trick somehow from Kafka standpoint, but I'm OK accepting your contribution since that is really out of our project scope to verify Kafka behavior.
| assertThat(((ConversionException) error.getPayload()).getMessage()) | ||
| .contains("Failed to convert to message"); | ||
| assertThat(((ConversionException) error.getPayload()).getRecords()).hasSize(2); | ||
| assertThat(restartAssignmentLatch.await(10, TimeUnit.SECONDS)).isTrue(); |
There was a problem hiding this comment.
This is somehow out of order of the whole test suite expectations.
I mean we probably should verify assignment first before checking for consumed record.
At the same time I don't understand why do we need to check this at all.
Looks like your main point is to ensure consumer is stopped until after we send records to the topic.
There was a problem hiding this comment.
yes, I have same doubts when writing this, I saw the first existing latch also do the verify after all the assertions passed.
I also ask myself do we need the second latch, and I think better have that, the batch mode test has two part, one is happy path , the other is error path , I think we can treat both equally? (keep second latch?)
There was a problem hiding this comment.
Sorry, I missed the part that we already checking for assignment for the first start.
Yes, it feels like we definitely do need it for the second one.
Thanks.
Merging!..
| assertThat(((ConversionException) error.getPayload()).getMessage()) | ||
| .contains("Failed to convert to message"); | ||
| assertThat(((ConversionException) error.getPayload()).getRecords()).hasSize(2); | ||
| assertThat(restartAssignmentLatch.await(10, TimeUnit.SECONDS)).isTrue(); |
There was a problem hiding this comment.
Sorry, I missed the part that we already checking for assignment for the first start.
Yes, it feels like we definitely do need it for the second one.
Thanks.
Merging!..
2 participants
By starting the adapter after sending messages, ensure a single poll retrieves the full batch messages.