Skip to content

MINOR: Add version check on enable-systest-events flag#1298

Closed
granders wants to merge 2 commits into
apache:trunkfrom
confluentinc:minor-systest-fix-versioned-console-consumer
Closed

MINOR: Add version check on enable-systest-events flag#1298
granders wants to merge 2 commits into
apache:trunkfrom
confluentinc:minor-systest-fix-versioned-console-consumer

Conversation

@granders
Copy link
Copy Markdown
Contributor

Recent patch adding enable-systest-events flag without any version check breaks all uses of versioned console consumer. E.g. upgrade tests, compatibility tests etc.

Added a check to only apply the flag if running 0.10.0 or greater.

@granders
Copy link
Copy Markdown
Contributor Author

@apovzner @ewencp
The issue was introduced in #1278 (which I also reviewed...)

Unfortunately I didn't think of this in the recent code review, but I noticed this in a subsequent branch build. We'll see a large chunk of test failures without this present patch.

cmd += " --formatter kafka.tools.LoggingMessageFormatter"

# enable systest events is only available in 0.10.0 and later
if node.version >= V_0_10_0_0:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's an issue for this patch, but I'm realizing that we may not have clearly defined when we bump version numbers pre-release. Here, we're clearly saying that we've already marked V_0_10_0_0, but that's actually a bit weird given that we're still on a dev version, right? Or does this not pass until we actually remove the dev0 suffix?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V_0_10_0_0 is defined independently of any kafkatest version bump. kafkatest version is the version of the kafkatest module, whereas an instance of KafkaVersion refers to a version of Kafka.

It's true that KafkaVersion inspects the init file to figure out what TRUNK should mean, but it strips any dev* suffix, so if we're in 0.10.0 branch, TRUNK actually equals V_0_10_0_0

@ewencp
Copy link
Copy Markdown
Contributor

ewencp commented Apr 30, 2016

@granders Good catch. Question about when exactly we do version bumping (and maybe about how exactly the comparison operator works?).

  1. I assume this patch needs double commit as it is relevant to the 0.10 branch, but I want to have people explicitly request double commits. This is the case, correct?
  2. Beyond the impact of this specific patch, what would have helped catch this? PR/branch builder? or does this require running against multiple branches to protect against? I want to figure out what the right process is so we catch these issues earlier.

@granders
Copy link
Copy Markdown
Contributor Author

granders commented May 2, 2016

@ewencp Thanks for review! here are a few answers:

version bumping... I don't think it really matters in this case given how comparison works? (see my previous version-related comments)

  1. Yes, we want double commit here
  2. PR/branch builder would've caught this no matter irrespective of the branch. Even a run of just the sanity checks would have caught this particular issue.

Running sanity checks here:
https://jenkins.confluent.io/job/system-test-kafka-branch-builder/425/console

Full build here:
https://jenkins.confluent.io/job/system-test-kafka-branch-builder-2/54/console

@granders
Copy link
Copy Markdown
Contributor Author

granders commented May 2, 2016

@ewencp Sanity checks now pass

@apovzner
Copy link
Copy Markdown
Contributor

apovzner commented May 2, 2016

@granders thanks for fixing this.

@ewencp
Copy link
Copy Markdown
Contributor

ewencp commented May 3, 2016

Full build had one failure, but its an existing transient issue. LGTM, thanks @granders!

@asfgit asfgit closed this in 5c47b9f May 3, 2016
asfgit pushed a commit that referenced this pull request May 3, 2016
Recent patch adding enable-systest-events flag without any version check breaks all uses of versioned console consumer. E.g. upgrade tests, compatibility tests etc.

Added a check to only apply the flag if running 0.10.0 or greater.

Author: Geoff Anderson <geoff@confluent.io>

Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>

Closes #1298 from granders/minor-systest-fix-versioned-console-consumer

(cherry picked from commit 5c47b9f)
Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
@granders granders deleted the minor-systest-fix-versioned-console-consumer branch May 19, 2016 00:14
gfodor pushed a commit to AltspaceVR/kafka that referenced this pull request Jun 3, 2016
Recent patch adding enable-systest-events flag without any version check breaks all uses of versioned console consumer. E.g. upgrade tests, compatibility tests etc.

Added a check to only apply the flag if running 0.10.0 or greater.

Author: Geoff Anderson <geoff@confluent.io>

Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>

Closes apache#1298 from granders/minor-systest-fix-versioned-console-consumer
mumrah pushed a commit to mumrah/kafka that referenced this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants