-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-4450: Add upgrade tests for 0.10.1 releases and rename TRUNK to CURRENT_BRANCH to reduce confusion. #2457
Conversation
… CURRENT_BRANCH to reduce confusion.
@ijuma Perhaps you can take a gander at this one. The bulk of the changes are just the renaming we discussed in the thread. I went with a slightly different name because the directory for the code and the name in the ducktape code are semi- tied together. Buried in the changes are the few test additions. I'm actually not that familiar with those tests and they include parameters that I was a bit unclear on why we have the set of parameterizations we have, so look carefully at those changes. I kicked off a test for the core system tests and it looks mostly ok here: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/691/consoleFull There are some failures, but some are known (the issue with ProduceConsumeValidate). The others I'm hoping someone can explain -- I'm not sure why a cluster ID in ZK (or lack thereof) would suddenly show up in these tests. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Looks good overall, left a few mostly minor comments and an explanation of the cluster id failure.
It may be worth updating the PR title to say DEV_BRANCH instead of CURRENT_BRANCH.
# that this check works with TRUNK | ||
# When running VerifiableProducer 0.8.X, both trunk version and 0.8.X should show up because of the way | ||
# verifiable producer pulls in some trunk directories into its classpath | ||
# that this check works with CURRENT_BRANCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be DEV_BRANCH?
else: | ||
return LooseVersion.__str__(self) | ||
|
||
|
||
def get_version(node=None): | ||
"""Return the version attached to the given node. | ||
Default to trunk if node or node.version is undefined (aka None) | ||
Default to CURRENT_BRANCH if node or node.version is undefined (aka None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEV_BRANCH
?
@@ -74,16 +74,16 @@ def check_node_or_version_helper(self): | |||
""" | |||
resolver = create_path_resolver(DummyContext()) | |||
|
|||
# Node with no version attribute should resolve to TRUNK | |||
# Node with no version attribute should resolve to CURRENT_BRANCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEV_BRANCH
@@ -62,6 +62,7 @@ def perform_upgrade(self, from_kafka_version, to_message_format_version=None): | |||
self.kafka.start_node(node) | |||
|
|||
@cluster(num_nodes=6) | |||
@parametrize(from_kafka_version=str(LATEST_0_10_1), to_message_format_version=None, compression_types=["lz4"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have a line for the old consumer as well.
Regarding your question about the cluster id, the following line needs to be conditional on from_kafka_version <= LATEST_0_10_0
:
assert self.zk.query("/cluster/id") is None
The code is just checking that the cluster id is generated after an upgrade from 0.10.0.x or lower to 0.10.1.x or higher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, thanks for the explanation.
@parametrize(producer_version=str(LATEST_0_9), consumer_version=str(DEV_BRANCH), compression_types=["snappy"], timestamp_type=None) | ||
@parametrize(producer_version=str(DEV_BRANCH), consumer_version=str(LATEST_0_9), compression_types=["snappy"], timestamp_type=str("CreateTime")) | ||
@parametrize(producer_version=str(DEV_BRANCH), consumer_version=str(DEV_BRANCH), compression_types=["snappy"], timestamp_type=str("LogAppendTime")) | ||
@parametrize(producer_version=str(LATEST_0_10_1), consumer_version=str(LATEST_0_10_1), compression_types=["snappy"],timestamp_type=str("LogAppendTime")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: space missing before timestamp_type
.
Addressed those issues and kicked of one final run to validate it: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/692/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM (assuming the tests pass).
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
System tests passed, merging to trunk and 0.10.2 branches. |
…CH to reduce confusion Author: Ewen Cheslack-Postava <me@ewencp.org> Reviewers: Ismael Juma <ismael@juma.me.uk> Closes #2457 from ewencp/kafka-4450-upgrade-tests
…CH to reduce confusion Author: Ewen Cheslack-Postava <me@ewencp.org> Reviewers: Ismael Juma <ismael@juma.me.uk> Closes apache#2457 from ewencp/kafka-4450-upgrade-tests
No description provided.