confluentinc / librdkafka

The Apache Kafka C/C++ library
Other
7.37k stars 3.11k forks source link

[KIP-951] Review comments for the tests #4767

Closed emasab closed 1 week ago

emasab commented 1 week ago

Mainly in the mock handlers there's need to use the generic rd_kafka_buf_write_tags to support more tags later without duplicating the code and the leader changes can be more than one so that's reflected in the new code.

I don't see necessary to have a separate file for the metadata update mock so kept it in the same metadata mock test file. Have changed a bit the test to be parametric for producer, consumer a single partition migration or two.

Given there's a fast metadata refresh that is still called, we shouldn't just verify that messages are Fetched or Produced to the new leader but that it's done before the metadata refresh.

One way it to slow down the metadata refresh but the mock rtts rd_kafka_mock_broker_push_request_error_rtts isn't used in the metadata mock handler. Tried to use it but there were issues because the metadata request is also used by the producer for the test.

Tried the interceptors too but the rd_kafka_interceptor_f_on_request_sent_t is called after sending the request. If it was before we could set a sleep there.

I think at the moment we cannot test more than that, automatically and in a predictable way.