[KIP-951] Review comments for the tests#4767
Merged
Emanuele Sabellico (emasab) merged 5 commits intomasterfrom Jun 21, 2024
Merged
[KIP-951] Review comments for the tests#4767Emanuele Sabellico (emasab) merged 5 commits intomasterfrom
Emanuele Sabellico (emasab) merged 5 commits intomasterfrom
Conversation
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 handler rd_kafka_mock_broker_push_request_error_rtts isn't used in the metadata mock handler. Tried that but the 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
Emanuele Sabellico (emasab)
changed the title
Closed
Emanuele Sabellico (emasab)
changed the base branch from
dev_kip_951_tests
to
master
Emanuele Sabellico (emasab)
dismissed
Anchit Jain (anchitj)’s stale review
The base branch was changed.
Anchit Jain (anchitj)
approved these changes
Jun 21, 2024
Emanuele Sabellico (emasab)
deleted the
dev_kip_951_tests_esabellico_review
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mainly in the mock handlers there's need to use the generic
rd_kafka_buf_write_tagsto 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_rttsisn'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_tis 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.