You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/09 23:13:05 UTC
[GitHub] [kafka] rondagostino opened a new pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
rondagostino opened a new pull request #10093:
URL: https://github.com/apache/kafka/pull/10093
We need to be able to run system tests with Raft-based metadata quorums -- both co-located brokers and controllers as well as remote controllers -- in addition to the ZooKepeer-based mode we run today. This PR adds this capability to `KafkaService` in a backwards-compatible manner as follows.
If no changes are made to existing system tests then they function as they always do -- they instantiate ZooKeeper, and Kafka will use ZooKeeper. A good test of this PR is therefore to run a full system test suite with no actual test changes and make sure everything runs as expected.
If we want to use a Raft-based metadata quorum we can do so by introducing a `metadata_quorum` argument to the test method and using `@matrix` to set it to the quorums we want to use for the various runs of the test. We then also have to skip creating a `ZooKeeperService` when the quorum is Raft-based.
For example, we would do the following:
```
from ducktape.mark import matrix
from kafkatest.services.kafka import KafkaService, quorum
```
```
def __init__(self, test_context):
super(TestVerifiableProducer, self).__init__(test_context)
self.zk = ZookeeperService(test_context, num_nodes=1) if quorum.for_test(test_context) == quorum.zk else None
self.kafka = KafkaService(test_context, num_nodes=1, zk=self.zk,
topics={"topic": {"partitions": 1, "replication-factor": 1}})
```
```
def setUp(self):
if self.zk:
self.zk.start()
self.kafka.start()
```
```
@cluster(num_nodes=3)
@matrix(producer_version=[str(DEV_BRANCH)], metadata_quorum=quorum.all)
def test_simple_run(self, producer_version=DEV_BRANCH, metadata_quorum=quorum.zk):
# the rest of the test logic remains unchanged
```
The above will end up running 3 separate tests: one with ZooKeeper, one with a co-located Raft-based controller, and once with a remote Raft-based controller.
If we want to set security protocols we could do this:
```
def setUp(self):
if self.zk:
self.zk.start()
# don't start Kafka here because we haven't configured security at this point
```
```
@cluster(num_nodes=3)
@matrix(producer_version=[str(DEV_BRANCH)], security_protocol=['PLAINTEXT', 'SSL'], metadata_quorum=quorum.all)
@cluster(num_nodes=4)
@matrix(producer_version=[str(DEV_BRANCH)], security_protocol=['SASL_SSL'], sasl_mechanism=['PLAIN', 'GSSAPI'],
metadata_quorum=quorum.all)
def test_simple_run(self, producer_version, security_protocol = 'PLAINTEXT', sasl_mechanism='PLAIN',
metadata_quorum=quorum.zk):
self.kafka.security_protocol = security_protocol
self.kafka.client_sasl_mechanism = sasl_mechanism
self.kafka.interbroker_security_protocol = security_protocol
self.kafka.interbroker_sasl_mechanism = sasl_mechanism
if self.kafka.quorum_info.using_raft:
controller_quorum = self.kafka.controller_quorum
controller_quorum.controller_security_protocol = security_protocol
controller_quorum.controller_sasl_mechanism = sasl_mechanism
controller_quorum.intercontroller_security_protocol = security_protocol
controller_quorum.intercontroller_sasl_mechanism = sasl_mechanism
# now we can start Kafka
self.kafka.start()
# the rest of the test logic remains unchanged
```
This PR does not update any tests -- those will come later after all the KIP-500 code is merged.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on a change in pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10093:
URL: https://github.com/apache/kafka/pull/10093#discussion_r573747683
##########
File path: config/raft-broker.properties
##########
@@ -0,0 +1,125 @@
+# 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.
+
+# see kafka.server.KafkaConfig for additional details and defaults
+
+############################# Server Basics #############################
+
+# The role of this server. Setting this puts us in kip-500 mode
+process.roles=broker
+
+# The node id associated with this instance's roles
+node.id=1001
Review comment:
> Since we've made the switch to a combined ID space it probably makes more sense to make [all node.id values] 1
Ah, good point. We should just start with 1 everywhere -- in the combined cluster, in the brokers-only cluster, and in the remote controller quorum cluster. There's no issue of overlap.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10093:
URL: https://github.com/apache/kafka/pull/10093#discussion_r573348107
##########
File path: config/raft-broker.properties
##########
@@ -0,0 +1,125 @@
+# 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.
+
+# see kafka.server.KafkaConfig for additional details and defaults
+
+############################# Server Basics #############################
+
+# The role of this server. Setting this puts us in kip-500 mode
+process.roles=broker
+
+# The node id associated with this instance's roles
+node.id=1001
Review comment:
Since we've made the switch to a combined ID space it probably makes more sense to make this 1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #10093:
URL: https://github.com/apache/kafka/pull/10093
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10093:
URL: https://github.com/apache/kafka/pull/10093#discussion_r573350073
##########
File path: tests/kafkatest/services/kafka/kafka.py
##########
@@ -171,35 +347,79 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
# e.g. brokers to deregister after a hard kill.
self.zk_session_timeout = zk_session_timeout
- self.port_mappings = {
- 'PLAINTEXT': KafkaListener('PLAINTEXT', 9092, 'PLAINTEXT', False),
- 'SSL': KafkaListener('SSL', 9093, 'SSL', False),
- 'SASL_PLAINTEXT': KafkaListener('SASL_PLAINTEXT', 9094, 'SASL_PLAINTEXT', False),
- 'SASL_SSL': KafkaListener('SASL_SSL', 9095, 'SASL_SSL', False),
+ broker_only_port_mappings = {
KafkaService.INTERBROKER_LISTENER_NAME:
- KafkaListener(KafkaService.INTERBROKER_LISTENER_NAME, 9099, None, False)
+ KafkaListener(KafkaService.INTERBROKER_LISTENER_NAME, config_property.FIRST_BROKER_PORT + 7, None, False)
Review comment:
what happens if we have more brokers than 7? There are tests like that.
Should we use something like 700 here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10093:
URL: https://github.com/apache/kafka/pull/10093#issuecomment-777114058
System test results:
```
SESSION REPORT (ALL TESTS)
ducktape version: 0.8.1
session_id: 2021-02-10--001
run time: 509 minutes 34.017 seconds
tests run: 725
passed: 425
failed: 103
ignored: 197
```
http://testing.confluent.io/confluent-kafka-branch-builder-system-test-results/?prefix=2021-02-10--001.1612998000--rondagostino--kip500_system_tests--b16fc75f6/report.html
Compared to a system test run from the day before:
```
SESSION REPORT (ALL TESTS)
ducktape version: 0.8.1
session_id: 2021-02-09--001
run time: 388 minutes 22.578 seconds
tests run: 729
passed: 453
failed: 79
ignored: 197
```
http://testing.confluent.io/confluent-kafka-branch-builder-system-test-results/?prefix=2021-02-09--001.1612926625--urbandan--KIP-635_GetOffsetShell--8e69e1dc5/report.html
There are lots more failures, but I looked and the failures are not due to the system tests not running. For example, an error message is `java.lang.Exception: UnsupportedVersionException: MetadataRequest versions older than 4 don't support the allowAutoTopicCreation field`.
So this PR is not affecting the **running** of the tests, which is all that matters.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10093:
URL: https://github.com/apache/kafka/pull/10093#discussion_r573348737
##########
File path: config/raft-controller.properties
##########
@@ -0,0 +1,124 @@
+# 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.
+
+# see kafka.server.KafkaConfig for additional details and defaults
+
+############################# Server Basics #############################
+
+# The role of this server. Setting this puts us in kip-500 mode
+process.roles=controller
+
+# The node id associated with this instance's roles
+node.id=3001
Review comment:
since we moved to a combined ID space we should just make this 1, I think
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10093:
URL: https://github.com/apache/kafka/pull/10093#discussion_r573348327
##########
File path: config/raft-broker.properties
##########
@@ -0,0 +1,125 @@
+# 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.
+
+# see kafka.server.KafkaConfig for additional details and defaults
+
+############################# Server Basics #############################
+
+# The role of this server. Setting this puts us in kip-500 mode
+process.roles=broker
+
+# The node id associated with this instance's roles
+node.id=1001
+
+# The connect string for the controller quorum
+controller.quorum.voters=3001@localhost:9093
+
+############################# Socket Server Settings #############################
+
+# The address the socket server listens on. It will get the value returned from
+# java.net.InetAddress.getCanonicalHostName() if not configured.
+# FORMAT:
+# listeners = listener_name://host_name:port
+# EXAMPLE:
+# listeners = PLAINTEXT://your.host.name:9092
+listeners=PLAINTEXT://localhost:9092
+inter.broker.listener.name=PLAINTEXT
+
+# Hostname and port the broker will advertise to producers and consumers. If not set,
+# it uses the value for "listeners" if configured. Otherwise, it will use the value
+# returned from java.net.InetAddress.getCanonicalHostName().
+advertised.listeners=PLAINTEXT://localhost:9092
+
+# Listener, host name, and port for the controller to advertise to the brokers. If
+# this server is a controller, this listener must be configured.
+controller.listener.names=CONTROLLER
+
+# Maps listener names to security protocols, the default is for them to be the same. See the config documentation for more details
+listener.security.protocol.map=CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT,SSL:SSL,SASL_PLAINTEXT:SASL_PLAINTEXT,SASL_SSL:SASL_SSL
+
+# The number of threads that the server uses for receiving requests from the network and sending responses to the network
+num.network.threads=3
+
+# The number of threads that the server uses for processing requests, which may include disk I/O
+num.io.threads=8
+
+# The send buffer (SO_SNDBUF) used by the socket server
+socket.send.buffer.bytes=102400
+
+# The receive buffer (SO_RCVBUF) used by the socket server
+socket.receive.buffer.bytes=102400
+
+# The maximum size of a request that the socket server will accept (protection against OOM)
+socket.request.max.bytes=104857600
+
+
+############################# Log Basics #############################
+
+# A comma separated list of directories under which to store log files
+log.dirs=/tmp/kafka-broker-logs
Review comment:
can we make this default to `/tmp/raft-broker-logs` to avoid confusion if people are trying out both new and old modes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on a change in pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10093:
URL: https://github.com/apache/kafka/pull/10093#discussion_r573743587
##########
File path: tests/kafkatest/services/kafka/kafka.py
##########
@@ -171,35 +347,79 @@ def __init__(self, context, num_nodes, zk, security_protocol=SecurityConfig.PLAI
# e.g. brokers to deregister after a hard kill.
self.zk_session_timeout = zk_session_timeout
- self.port_mappings = {
- 'PLAINTEXT': KafkaListener('PLAINTEXT', 9092, 'PLAINTEXT', False),
- 'SSL': KafkaListener('SSL', 9093, 'SSL', False),
- 'SASL_PLAINTEXT': KafkaListener('SASL_PLAINTEXT', 9094, 'SASL_PLAINTEXT', False),
- 'SASL_SSL': KafkaListener('SASL_SSL', 9095, 'SASL_SSL', False),
+ broker_only_port_mappings = {
KafkaService.INTERBROKER_LISTENER_NAME:
- KafkaListener(KafkaService.INTERBROKER_LISTENER_NAME, 9099, None, False)
+ KafkaListener(KafkaService.INTERBROKER_LISTENER_NAME, config_property.FIRST_BROKER_PORT + 7, None, False)
Review comment:
This is calculating the port for the inter-broker listener. The mapping from security protocol to port is as shown below. This change is simply a rewrite of an existing hard-coded number (9099) to a calculation (9092 + 7). There is no issue regardless of the number of brokers.
```
Client listener when using SecurityConfig.PLAINTEXT ==> 9092
Client listener when using SecurityConfig.SS ==> 9093
Client listener when using SecurityConfig.SASL_PLAINTEXT ==> 9094
Client listener when using SecurityConfig.SASL_SSL ==> 9095
Inter-broker listener (and whatever security protocol it is set to) ==> 9092 + 7 = 9099
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on a change in pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #10093:
URL: https://github.com/apache/kafka/pull/10093#discussion_r573744601
##########
File path: config/raft-broker.properties
##########
@@ -0,0 +1,125 @@
+# 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.
+
+# see kafka.server.KafkaConfig for additional details and defaults
+
+############################# Server Basics #############################
+
+# The role of this server. Setting this puts us in kip-500 mode
+process.roles=broker
+
+# The node id associated with this instance's roles
+node.id=1001
+
+# The connect string for the controller quorum
+controller.quorum.voters=3001@localhost:9093
+
+############################# Socket Server Settings #############################
+
+# The address the socket server listens on. It will get the value returned from
+# java.net.InetAddress.getCanonicalHostName() if not configured.
+# FORMAT:
+# listeners = listener_name://host_name:port
+# EXAMPLE:
+# listeners = PLAINTEXT://your.host.name:9092
+listeners=PLAINTEXT://localhost:9092
+inter.broker.listener.name=PLAINTEXT
+
+# Hostname and port the broker will advertise to producers and consumers. If not set,
+# it uses the value for "listeners" if configured. Otherwise, it will use the value
+# returned from java.net.InetAddress.getCanonicalHostName().
+advertised.listeners=PLAINTEXT://localhost:9092
+
+# Listener, host name, and port for the controller to advertise to the brokers. If
+# this server is a controller, this listener must be configured.
+controller.listener.names=CONTROLLER
+
+# Maps listener names to security protocols, the default is for them to be the same. See the config documentation for more details
+listener.security.protocol.map=CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT,SSL:SSL,SASL_PLAINTEXT:SASL_PLAINTEXT,SASL_SSL:SASL_SSL
+
+# The number of threads that the server uses for receiving requests from the network and sending responses to the network
+num.network.threads=3
+
+# The number of threads that the server uses for processing requests, which may include disk I/O
+num.io.threads=8
+
+# The send buffer (SO_SNDBUF) used by the socket server
+socket.send.buffer.bytes=102400
+
+# The receive buffer (SO_RCVBUF) used by the socket server
+socket.receive.buffer.bytes=102400
+
+# The maximum size of a request that the socket server will accept (protection against OOM)
+socket.request.max.bytes=104857600
+
+
+############################# Log Basics #############################
+
+# A comma separated list of directories under which to store log files
+log.dirs=/tmp/kafka-broker-logs
Review comment:
> can we make this default to /tmp/raft-broker-logs [instead of /tmp/kafka-broker-logs]
I changed all paths, so now we specify /tmp/raft-{broker,controller,combined}-logs for the paths in the 3 separate properties files.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a change in pull request #10093: MINOR: Support Raft-based metadata quorums in system tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on a change in pull request #10093:
URL: https://github.com/apache/kafka/pull/10093#discussion_r573348782
##########
File path: config/raft-controller.properties
##########
@@ -0,0 +1,124 @@
+# 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.
+
+# see kafka.server.KafkaConfig for additional details and defaults
+
+############################# Server Basics #############################
+
+# The role of this server. Setting this puts us in kip-500 mode
+process.roles=controller
+
+# The node id associated with this instance's roles
+node.id=3001
+
+# The connect string for the controller quorum
+controller.quorum.voters=3001@localhost:9093
Review comment:
same comment here-- let's just make it 1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org