MINOR: Change ConnectDistributed system test to use KafkaVersion instead of string#7023
Conversation
|
The system test run of Given that all of these system tests failed before this change, we should still be able to merge this and backport at least to the 2.3 branch, and potentially back to the |
rajinisivaram
left a comment
rajinisivaram
left a comment
There was a problem hiding this comment.
@rhauch Thanks for the PR. Left a note about updating the caller instead. We use KafkaVersion in all other cases as argument to KafkaService, but the doc in KafkaService is out-of-date. It will be good to update that as well to avoid confusion.
| self.kafka = KafkaService(self.test_context, self.num_brokers, self.zk, | ||
| security_protocol=security_protocol, interbroker_security_protocol=security_protocol, | ||
| topics=self.topics, version=broker_version, | ||
| topics=self.topics, version=KafkaVersion(broker_version), |
There was a problem hiding this comment.
I think it would be better to update callers of setup_services to use KafkaVersion objects since at the moment they are converting to string. The default value DEV_BRANCH used here is already a KafkaVersion instance. Looks like we only need to update the parameters in this method:
rajinisivaram
force-pushed
the
fix-connect-distributed-system-tests
branch
from
6f852bb to
96f88c1
Compare
|
System test run with the updated PR (https://jenkins.confluent.io/job/system-test-kafka-branch-builder/2767/) had the same two failures from Randall's test run yesterday with his PR. Randall mentioned that these looked like intermittent failures. But I am not sure the compatibility tests would ever have worked against a 0.9.0.1 broker. The logs show that the Connect client sent an ApiVersions request that the 0.9.0.1 broker didn't recognize. So I looked at the last successful run and those were working since all the compatibility tests were always running against a |
ijuma
left a comment
ijuma
left a comment
There was a problem hiding this comment.
LGTM, please include the updated message in the commit with an explanation why we removed 0.9.x.
|
@ijuma Thanks for the review, merging to trunk with detailed commit message. |
… compatibility test (apache#7023) Connect tests were using String version for KafkaService instead of the expected KafkaVersion object. This broke due to recent changes to KafkaVersion. It turns out that the tests with String version were running compatibility tests against `dev` brokers rather than the older broker versions they were expecting to run against. When version was fixed, tests using 0.9.0.1 brokers started failing since new clients are not compatible with 0.9.0.1 brokers. So this PR fixes version parameter and removes the two tests against 0.9.0.1 brokers. Reviewers: Ismael Juma <ismael@juma.me.uk>, Rajini Sivaram <rajinisivaram@googlemail.com>
… compatibility test (apache#7023) Connect tests were using String version for KafkaService instead of the expected KafkaVersion object. This broke due to recent changes to KafkaVersion. It turns out that the tests with String version were running compatibility tests against `dev` brokers rather than the older broker versions they were expecting to run against. When version was fixed, tests using 0.9.0.1 brokers started failing since new clients are not compatible with 0.9.0.1 brokers. So this PR fixes version parameter and removes the two tests against 0.9.0.1 brokers. Reviewers: Ismael Juma <ismael@juma.me.uk>, Rajini Sivaram <rajinisivaram@googlemail.com>
(apache#7023) exposed an incompatibility between Kafka <=0.9 and Connect >0.9, in which the broker does not recognize a request for ApiVersions. For trunk and 2.4, this test case was removed rather than the issue addressed. This effectively backports the other half of (apache#7023) which was left out of (apache#7791). Signed-off-by: Greg Harris <gregh@confluent.io>
…DistributedTest (#8035) (#7023) exposed an incompatibility between Kafka <=0.9 and Connect >0.9, in which the broker does not recognize a request for ApiVersions. For trunk and 2.4, this test case was removed rather than the issue addressed. This effectively backports the other half of (#7023) which was left out of (#7791). Signed-off-by: Greg Harris <gregh@confluent.io> Author: Greg Harris <gregh@confluent.io> Reviewers: Randall Hauch <rhauch@gmail.com>, Andrew Choi <andchoi@linkedin.com>
…DistributedTest (#8035) (#7023) exposed an incompatibility between Kafka <=0.9 and Connect >0.9, in which the broker does not recognize a request for ApiVersions. For trunk and 2.4, this test case was removed rather than the issue addressed. This effectively backports the other half of (#7023) which was left out of (#7791). Signed-off-by: Greg Harris <gregh@confluent.io> Author: Greg Harris <gregh@confluent.io> Reviewers: Randall Hauch <rhauch@gmail.com>, Andrew Choi <andchoi@linkedin.com>
…DistributedTest (#8035) (#7023) exposed an incompatibility between Kafka <=0.9 and Connect >0.9, in which the broker does not recognize a request for ApiVersions. For trunk and 2.4, this test case was removed rather than the issue addressed. This effectively backports the other half of (#7023) which was left out of (#7791). Signed-off-by: Greg Harris <gregh@confluent.io> Author: Greg Harris <gregh@confluent.io> Reviewers: Randall Hauch <rhauch@gmail.com>, Andrew Choi <andchoi@linkedin.com>
3 participants
Recently, #7000 added a method to the
KafkaVersionclass used in the Kafka system tests. Until then, ConnectDistributed tests passed a string as the version, and this happened to work since no methods were required. But theKafkaVersion.support_named_listeners()method added in #7000 and required intests/kafkatest/services/kafka/templates/kafka.propertiesappears to have broken the ConnectDistributed system tests. Changing the latter to useKafkaVersioninstead of string fixes the system tests.I'll run a system test build before this should be merged to
trunkand at least2.3(ideally back to1.0).Committer Checklist (excluded from commit message)