MINOR: fix client_compatibility_features_test.py by chia7712 · Pull Request #10292 · apache/kafka

chia7712 · 2021-03-10T15:43:02Z

related to #10105

kafka nodes are set to older version so resolved script path is linked to older assembly. the 0.10.x versions do not have org.apache.kafka.tools.ClientCompatibilityTest so the test gets failed.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

chia7712 · 2021-03-10T17:21:15Z

oh, it tests raft also. will update code to fix that.

rondagostino · 2021-03-11T19:03:38Z

+            # this is supported by zkBroker currently
+            features["describe-acls-supported"] = False


Nice -- better than disabling the test for Raft-based quorums. Perhaps add to the comment to indicate that this check/disabling is only necessary due to the fact that we are in early access mode with KIP-500 and we should remove the special casing when that his fully implemented? Aside from that, LGTM -- thank you for the PR!

will copy that.

rondagostino · 2021-03-11T19:04:11Z

This needs to be cherry-picked to 2.8.

chia7712 · 2021-03-14T08:14:28Z

@vvcephei Do you have free time to take a look at this PR? This issue happens in 2.8 also so it would be nice to be included by 2.8.0 :)

cmccabe

Wait, can you explain why we need this change now and we didn't need it before?

And if we always want to use the development version of the features test, it seems like this doesn't do that in the KIP-500 case, right?

rondagostino · 2021-03-16T22:06:02Z

I broke this with https://github.com/apache/kafka/pull/10105/files#diff-84e14a0909d232b70f0a957ded161cd077d4dc1d069bbaab8e1bacc8dd2e0572L84-R85. The test used to always use the ZooKeeper node, and I changed it to use the Kafka node not realizing that the two distribution versions would be different. We want it to always use at least version 2.7, which we can get by using either the ZooKeeper node for the Zookeeper case or the Kafka node for the Raft case.

cmccabe · 2021-03-16T23:05:34Z

@rondagostino : thanks for the explanation.

It's a bit weird to use the devel version of the tool when ZK is enabled, and the old version when it's not. I think this will lead to some odd issues down the road.

@chia7712 : Can we just use a separate node for the client, unconditionally? Then that separate node can always have the devel software.

Or, I suppose, we could change the path that the compat tool is invoked via (maybe that's annoying, though...)

rondagostino · 2021-03-16T23:28:29Z

use the devel version of the tool when ZK is enabled, and the old version when it's not

The code as amended in this PR will use the devel version when ZooKeeper is enabled (which is the way it always was except after I changed/broke it) and then, when Zookeeper is not enabled -- which is a new case -- it will use the Kafka version, which by definition will be >= 2.8.

rondagostino · 2021-03-16T23:42:59Z

Can we just use a separate node for the client, unconditionally

We don't have a Service for it, and we currently invoke it in a node that we allocate for the test. With no ZooKeeper all we have is Kafka (version >= 2.8, of course).

I could see a situation where we add functionality to org.apache.kafka.tools.ClientCompatibilityTest in some future version and then we try to run that against Kafka v2.8 -- and then it'll go BOOM! as you suggest.

rondagostino · 2021-03-17T00:59:02Z

-        # Run the compatibility test on the first Kafka node.
-        node = self.kafka.nodes[0]
+        if self.zk:
+            # kafka nodes are set to older version so resolved script path is linked to older assembly.
+            # run the compatibility test on the first zk node to get script path linked to latest(dev) assembly.
+            node = self.zk.nodes[0]
+        else:
+            node = self.kafka.nodes[0]


We can use the dev version of the tool on the Kafka node via code like this:

node = self.kafka.nodes[0] cmd = ("%s org.apache.kafka.tools.ClientCompatibilityTest " "--bootstrap-server %s " "--num-cluster-nodes %d " "--topic %s " % (self.dev_script_path, self.kafka.bootstrap_servers(), len(self.kafka.nodes), list(self.topics.keys())[0]))

And then further down we can define the DEV script path like this:

# Always use the latest version of org.apache.kafka.tools.ClientCompatibilityTest # so store away the path to the DEV version before we set the Kafka version self.dev_script_path = self.kafka.path.script("kafka-run-class.sh", self.kafka.nodes[0]) self.kafka.set_version(KafkaVersion(broker_version))

I tested this locally and it solves the problem.

chia7712 · 2021-03-17T02:24:44Z

@cmccabe @rondagostino thanks for all your comments (and nice explanation)! I have update code according to reviews. please take a look.

rondagostino · 2021-03-17T14:41:47Z

+        self.dev_script_path = self.kafka.path.script("kafka-run-class.sh", self.kafka.nodes[0])

    def invoke_compatibility_program(self, features):
-        # Run the compatibility test on the first Kafka node.


nit: no need to remove this comment. Otherwise LGTM -- thank you again!

will copy that.

cmccabe · 2021-03-17T17:23:35Z

Thanks, @chia7712 !

Reviewers: Colin Patrick McCabe <cmccabe@confluent.io>, Ron Dagostino <rdagostino@confluent.io>

chia7712 · 2021-03-17T17:30:53Z

push to trunk and 2.8

Conflicts: * build.gradle: keep `dependencySubstitution` Confluent addition in `resolutionStrategy` and take upstream changes. Commits: * apache-github/trunk: KAFKA-12503: inform threads to resize their cache instead of doing so for them (apache#10356) KAFKA-10697: Remove ProduceResponse.responses (apache#10332) MINOR: Exclude KIP-500.md from rat check (apache#10354) MINOR: Move `configurations.all` to be a child of `allprojects` (apache#10349) MINOR: Remove use of `NoSuchElementException` in `KafkaMetadataLog` (apache#10344) MINOR: Start the broker-to-controller channel for request forwarding (apache#10340) KAFKA-12382: add a README for KIP-500 (apache#10227) MINOR: Fix BaseHashTable sizing (apache#10334) KAFKA-10357: Add setup method to internal topics (apache#10317) MINOR: remove redundant null check when testing specified type (apache#10314) KAFKA-12293: Remove JCenter from buildscript and delete buildscript.gradle KAFKA-12491: Make rocksdb an `api` dependency for `streams` (apache#10341) KAFKA-12454: Add ERROR logging on kafka-log-dirs when given brokerIds do not exist in current kafka cluster (apache#10304) KAFKA-12459; Use property testing library for raft event simulation tests (apache#10323) MINOR: fix failing ZooKeeper system tests (apache#10297) MINOR: fix client_compatibility_features_test.py (apache#10292)

chia7712 requested review from cmccabe and ijuma March 10, 2021 15:43

MINOR: fix client_compatibility_features_test.py

11feb2d

chia7712 force-pushed the MINOR-10292 branch from d6a0c67 to 11feb2d Compare March 10, 2021 18:09

rondagostino mentioned this pull request Mar 11, 2021

MINOR: Disable client compatibility system test for Raft quorums #10298

Closed

3 tasks

rondagostino reviewed Mar 11, 2021

View reviewed changes

chia7712 added 2 commits March 12, 2021 11:30

Merge branch 'trunk' into MINOR-10292

7e49684

polish comments

55197e7

cmccabe approved these changes Mar 16, 2021 •

edited

Loading

View reviewed changes

cmccabe requested changes Mar 16, 2021

View reviewed changes

rondagostino reviewed Mar 17, 2021

View reviewed changes

chia7712 added 2 commits March 17, 2021 10:21

address review comment

a15166b

Merge branch 'trunk' into MINOR-10292

b724ffd

rondagostino reviewed Mar 17, 2021

View reviewed changes

chia7712 added 2 commits March 17, 2021 22:43

Merge branch 'trunk' into MINOR-10292

0c22ff6

restore useful comment

f541425

cmccabe approved these changes Mar 17, 2021

View reviewed changes

chia7712 merged commit 3288db5 into apache:trunk Mar 17, 2021

cmccabe pushed a commit that referenced this pull request Mar 17, 2021

MINOR: fix client_compatibility_features_test.py (#10292)

5d04349

Reviewers: Colin Patrick McCabe <cmccabe@confluent.io>, Ron Dagostino <rdagostino@confluent.io>

chia7712 deleted the MINOR-10292 branch March 25, 2024 15:21

		# this is supported by zkBroker currently
		features["describe-acls-supported"] = False

Conversation

chia7712 commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 commented Mar 10, 2021

Uh oh!

rondagostino Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

chia7712 Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

rondagostino commented Mar 11, 2021

Uh oh!

chia7712 commented Mar 14, 2021

Uh oh!

Uh oh!

cmccabe left a comment

Choose a reason for hiding this comment

Uh oh!

rondagostino commented Mar 16, 2021

Uh oh!

cmccabe commented Mar 16, 2021

Uh oh!

rondagostino commented Mar 16, 2021

Uh oh!

rondagostino commented Mar 16, 2021

Uh oh!

rondagostino Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

chia7712 Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Mar 17, 2021

Uh oh!

rondagostino Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

chia7712 Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Mar 17, 2021

Uh oh!

chia7712 commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chia7712 commented Mar 10, 2021 •

edited

Loading