-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-20629: Add ducktape system tests for topology description plugin #22769
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
base: trunk
Are you sure you want to change the base?
Changes from 9 commits
02ad10f
be35f9e
897509c
de2749f
d88e1ac
22caa6b
e9e6251
ecd4ea8
83289f0
7a0e8f4
6d24778
cc5bad7
d767996
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.kafka.streams.tests; | ||
|
|
||
| import org.apache.kafka.common.serialization.Serdes; | ||
| import org.apache.kafka.common.utils.Utils; | ||
| import org.apache.kafka.common.utils.internals.Exit; | ||
| import org.apache.kafka.streams.KafkaStreams; | ||
| import org.apache.kafka.streams.StreamsBuilder; | ||
| import org.apache.kafka.streams.StreamsConfig; | ||
| import org.apache.kafka.streams.errors.StreamsUncaughtExceptionHandler; | ||
| import org.apache.kafka.streams.kstream.Consumed; | ||
| import org.apache.kafka.streams.kstream.Produced; | ||
|
|
||
| import java.io.IOException; | ||
| import java.time.Duration; | ||
| import java.util.Locale; | ||
| import java.util.Properties; | ||
|
|
||
| public class TopologyDescriptionPluginSystemTest { | ||
|
|
||
| private static final String APPLICATION_ID = "kafka-streams-system-test-topology-description-plugin"; | ||
| private static final String SOURCE_TOPIC = "topologyDescriptionPluginSource"; | ||
| private static final String SINK_TOPIC = "topologyDescriptionPluginSink"; | ||
|
|
||
| public static void main(final String[] args) throws IOException { | ||
| if (args.length != 1) { | ||
| System.err.println("TopologyDescriptionPluginSystemTest expects one parameter: propFile"); | ||
| Exit.exit(1); | ||
| } | ||
|
|
||
| System.out.println("TopologyDescriptionPluginSystemTest starting"); | ||
|
|
||
| final String propFileName = args[0]; | ||
| final Properties streamsProperties = Utils.loadProps(propFileName); | ||
| final String bootstrap = streamsProperties.getProperty(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG); | ||
|
|
||
| if (bootstrap == null) { | ||
| System.err.println("No bootstrap kafka servers specified in " + StreamsConfig.BOOTSTRAP_SERVERS_CONFIG); | ||
| Exit.exit(1); | ||
| } | ||
|
|
||
| streamsProperties.put(StreamsConfig.APPLICATION_ID_CONFIG, APPLICATION_ID); | ||
| streamsProperties.put(StreamsConfig.DEFAULT_KEY_SERDE_CLASS_CONFIG, Serdes.String().getClass()); | ||
| streamsProperties.put(StreamsConfig.DEFAULT_VALUE_SERDE_CLASS_CONFIG, Serdes.String().getClass()); | ||
|
|
||
| final StreamsBuilder builder = new StreamsBuilder(); | ||
| builder.<String, String>stream(SOURCE_TOPIC, Consumed.with(Serdes.String(), Serdes.String())) | ||
| .mapValues(value -> value.toLowerCase(Locale.ROOT)) | ||
| .groupByKey() | ||
| .count() | ||
| .toStream() | ||
| .mapValues(count -> Long.toString(count)) | ||
| .to(SINK_TOPIC, Produced.with(Serdes.String(), Serdes.String())); | ||
|
|
||
| final KafkaStreams streams = new KafkaStreams(builder.build(), streamsProperties); | ||
| streams.setUncaughtExceptionHandler(e -> { | ||
| System.err.println("FATAL: An unexpected exception " + e); | ||
| System.err.flush(); | ||
| return StreamsUncaughtExceptionHandler.StreamThreadExceptionResponse.SHUTDOWN_CLIENT; | ||
| }); | ||
|
|
||
| System.out.println("Start Kafka Streams"); | ||
| streams.start(); | ||
| System.out.println("STREAMS-STARTED"); | ||
| System.out.flush(); | ||
|
|
||
| Exit.addShutdownHook("streams-shutdown-hook", () -> { | ||
| streams.close(Duration.ofSeconds(30)); | ||
| System.out.println("TopologyDescriptionPluginSystemTest closed"); | ||
| System.out.flush(); | ||
| }); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,10 +19,13 @@ | |
| from ducktape.tests.test import Test | ||
| from ducktape.utils.util import wait_until | ||
| from kafkatest.services.kafka import KafkaService, quorum | ||
| from kafkatest.services.streams import StreamsBrokerCompatibilityService | ||
| from kafkatest.services.streams import ( | ||
| INMEMORY_TOPOLOGY_DESCRIPTION_PLUGIN_CLASS, | ||
| StreamsBrokerCompatibilityService, | ||
| ) | ||
| from kafkatest.services.verifiable_consumer import VerifiableConsumer | ||
| from kafkatest.version import LATEST_3_0, LATEST_3_1, LATEST_3_2, LATEST_3_3, LATEST_3_4, LATEST_3_5, LATEST_3_6, \ | ||
| LATEST_3_7, LATEST_3_8, LATEST_3_9, LATEST_4_0, LATEST_4_1, LATEST_4_2, LATEST_4_3, KafkaVersion | ||
| LATEST_3_7, LATEST_3_8, LATEST_3_9, LATEST_4_0, LATEST_4_1, LATEST_4_2, LATEST_4_3, DEV_BRANCH, KafkaVersion | ||
|
|
||
|
|
||
| class StreamsBrokerCompatibility(Test): | ||
|
|
@@ -45,7 +48,8 @@ def __init__(self, test_context): | |
| }, | ||
| server_prop_overrides=[ | ||
| ["transaction.state.log.replication.factor", "1"], | ||
| ["transaction.state.log.min.isr", "1"] | ||
| ["transaction.state.log.min.isr", "1"], | ||
| ["group.streams.topology.description.plugin.class", INMEMORY_TOPOLOGY_DESCRIPTION_PLUGIN_CLASS] | ||
| ]) | ||
| self.consumer = VerifiableConsumer(test_context, | ||
| 1, | ||
|
|
@@ -58,9 +62,10 @@ def __init__(self, test_context): | |
| @matrix(broker_version=[str(LATEST_3_0),str(LATEST_3_1),str(LATEST_3_2),str(LATEST_3_3), | ||
| str(LATEST_3_4),str(LATEST_3_5),str(LATEST_3_6),str(LATEST_3_7), | ||
| str(LATEST_3_8),str(LATEST_3_9),str(LATEST_4_0),str(LATEST_4_1), | ||
| str(LATEST_4_2),str(LATEST_4_3)], | ||
| str(LATEST_4_2),str(LATEST_4_3),str(DEV_BRANCH)], | ||
| metadata_quorum=[quorum.combined_kraft] | ||
| ) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stray whitespace-only line added between the |
||
| def test_compatible_brokers_eos_disabled(self, broker_version, metadata_quorum): | ||
| self.kafka.set_version(KafkaVersion(broker_version)) | ||
| self.kafka.start() | ||
|
|
@@ -81,7 +86,7 @@ def test_compatible_brokers_eos_disabled(self, broker_version, metadata_quorum): | |
| @matrix(broker_version=[str(LATEST_3_0),str(LATEST_3_1),str(LATEST_3_2),str(LATEST_3_3), | ||
| str(LATEST_3_4),str(LATEST_3_5),str(LATEST_3_6),str(LATEST_3_7), | ||
| str(LATEST_3_8),str(LATEST_3_9),str(LATEST_4_0),str(LATEST_4_1), | ||
| str(LATEST_4_2),str(LATEST_4_3)], | ||
| str(LATEST_4_2),str(LATEST_4_3),str(DEV_BRANCH)], | ||
| metadata_quorum=[quorum.combined_kraft]) | ||
| def test_compatible_brokers_eos_v2_enabled(self, broker_version, metadata_quorum): | ||
| self.kafka.set_version(KafkaVersion(broker_version)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| # Licensed to the Apache Software Foundation (ASF) under one or more | ||
| # contributor license agreements. See the NOTICE file distributed with | ||
| # this work for additional information regarding copyright ownership. | ||
| # The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| # (the "License"); you may not use this file except in compliance with | ||
| # the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| import time | ||
|
|
||
| from ducktape.mark import matrix | ||
| from ducktape.mark.resource import cluster | ||
| from ducktape.tests.test import Test | ||
| from kafkatest.services.kafka import KafkaService, quorum | ||
| from kafkatest.services.streams import ( | ||
| INMEMORY_TOPOLOGY_DESCRIPTION_PLUGIN_CLASS, | ||
| StreamsTopologyDescriptionPluginService, | ||
| ) | ||
|
|
||
|
|
||
| class StreamsTopologyDescriptionPluginTest(Test): | ||
|
|
||
| PUSH_REQUESTED_LOG = "Broker requested topology description push" | ||
| PUSH_SENDING_LOG = "Sending topology description for group" | ||
| PUSH_SUCCESS_LOG = "Topology description pushed successfully" | ||
|
|
||
| SOURCE_TOPIC = "topologyDescriptionPluginSource" | ||
| SINK_TOPIC = "topologyDescriptionPluginSink" | ||
|
|
||
| def __init__(self, test_context): | ||
| super(StreamsTopologyDescriptionPluginTest, self).__init__(test_context=test_context) | ||
| self.topics = { | ||
| self.SOURCE_TOPIC: {"partitions": 1, "replication-factor": 1}, | ||
| self.SINK_TOPIC: {"partitions": 1, "replication-factor": 1}, | ||
| } | ||
|
|
||
| def setup_kafka(self, plugin_enabled): | ||
| server_prop_overrides = [ | ||
| ["group.streams.min.session.timeout.ms", "10000"], | ||
| ["group.streams.session.timeout.ms", "10000"], | ||
| ] | ||
| if plugin_enabled: | ||
| server_prop_overrides.append( | ||
| ["group.streams.topology.description.plugin.class", INMEMORY_TOPOLOGY_DESCRIPTION_PLUGIN_CLASS]) | ||
| self.kafka = KafkaService( | ||
| self.test_context, | ||
| num_nodes=1, | ||
| zk=None, | ||
| topics=self.topics, | ||
| use_streams_groups=True, | ||
| server_prop_overrides=server_prop_overrides, | ||
| ) | ||
| self.kafka.start() | ||
| self.kafka.run_features_command("upgrade", "streams.version", 1) | ||
|
|
||
| @cluster(num_nodes=2) | ||
| @matrix(metadata_quorum=[quorum.combined_kraft]) | ||
| def test_topology_description_available_with_plugin(self, metadata_quorum): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are follow-up scenarios planned for the plugin lifecycle? Three come to mind that the in-memory plugin makes observable with exactly the infrastructure this PR adds:
No need to grow this PR — just wondering whether these are on the part-2 list.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the ideas! I will append more scenarios in a followup PR. |
||
| """ | ||
| Test the situation when the broker has the topology description plugin configured | ||
| and the client pushes by default. The broker should solicit a push and the client | ||
| should complete it successfully. | ||
| """ | ||
| self.setup_kafka(plugin_enabled=True) | ||
| processor = StreamsTopologyDescriptionPluginService(self.test_context, self.kafka) | ||
| with processor.node.account.monitor_log(processor.LOG_FILE) as monitor: | ||
| processor.start() | ||
| monitor.wait_until(self.PUSH_SUCCESS_LOG, | ||
| timeout_sec=120, | ||
| err_msg="Streams client did not log a successful topology description push") | ||
| processor.stop() | ||
|
|
||
| @cluster(num_nodes=2) | ||
| @matrix(metadata_quorum=[quorum.combined_kraft]) | ||
| def test_topology_description_not_stored_when_client_opts_out(self, metadata_quorum): | ||
| """ | ||
| Test the situation when the broker has the topology description plugin configured | ||
| but the client opts out via topology.description.push.enabled=false. The client | ||
| should never send a topology description even though the broker solicits one. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit on the docstring: "even though the broker solicits one" is true broker-side, but it isn't observable in this test. When the client opts out,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix: added additional explanation indicating the "broker requested topology description push" log never appears; an additional assertion on this is also added. |
||
| """ | ||
| self.setup_kafka(plugin_enabled=True) | ||
| processor = StreamsTopologyDescriptionPluginService( | ||
| self.test_context, self.kafka, topology_description_push_enabled=False) | ||
| processor.start() | ||
| time.sleep(30) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The two negative tests can pass vacuously: Consider waiting for a positive liveness signal before doing the negative grep — e.g. wait for the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix: added assertions to wait for client to join the group. |
||
| sent = processor.node.account.ssh_capture( | ||
| "grep -c '%s' %s || true" % (self.PUSH_SENDING_LOG, processor.LOG_FILE), | ||
| allow_fail=False) | ||
| assert int(next(sent).strip()) == 0, \ | ||
| "Client sent a topology description despite topology.description.push.enabled=false" | ||
| pushed = processor.node.account.ssh_capture( | ||
| "grep -c '%s' %s || true" % (self.PUSH_SUCCESS_LOG, processor.LOG_FILE), | ||
| allow_fail=False) | ||
| assert int(next(pushed).strip()) == 0, \ | ||
| "Client logged a successful push despite topology.description.push.enabled=false" | ||
| processor.stop() | ||
|
|
||
| @cluster(num_nodes=2) | ||
| @matrix(metadata_quorum=[quorum.combined_kraft]) | ||
| def test_topology_description_not_stored_without_plugin(self, metadata_quorum): | ||
| """ | ||
| Test the situation when no topology description plugin is configured on the broker. | ||
| The broker should never solicit a topology push from the client. | ||
| """ | ||
| self.setup_kafka(plugin_enabled=False) | ||
| processor = StreamsTopologyDescriptionPluginService(self.test_context, self.kafka) | ||
| processor.start() | ||
| time.sleep(30) | ||
| # count how many lines in streams.log contain Broker requested topology description push | ||
| solicited = processor.node.account.ssh_capture( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only checks Broker requested topology description push
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix: added the 2 client-side assertions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. StreamsGroupHeartbeatRequestManager.java:657-660: When
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I added a broker log after the config is solicit on the broker side, and an additional check in the system test for its appearance. |
||
| "grep -c '%s' %s || true" % (self.PUSH_REQUESTED_LOG, processor.LOG_FILE), | ||
| allow_fail=False) | ||
| assert int(next(solicited).strip()) == 0, \ | ||
| "Broker solicited a topology push even though no plugin was configured" | ||
| processor.stop() | ||
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.
The PR description says the plugin is enabled "for DEV_VERSION", but this override applies to every broker version in the matrix. It happens to work because
group.streams.topology.description.plugin.classonly exists on trunk (not in 4.3 or earlier), so older brokers just log an unknown-config warning and ignore it — but that's subtle enough to deserve an inline comment.Also, since
StreamsBrokerCompatibilityServiceruns with the classic protocol (no streams groups exist in this test), the plugin is instantiated but never exercised. Could you clarify what this addition is meant to verify — that a DEV broker with the plugin configured doesn't affect classic-protocol streams apps? A short comment stating that intent would help.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.
Fix: I added comments about this config: 1) it is only in actual effect when
broker_version>4.32) This test is only using classic group so the plugin is not taking effect.