PR review changes: Sorted Imports in alpha order, made group description response processing more readable, enhanced tests

Author: Apurva Telang

kafka/admin/client.py

@@ -6,9 +6,11 @@
 import socket
 
 from . import ConfigResourceType
-from kafka.vendor import six
 
+from kafka.admin.acl_resource import ACLOperation, ACLPermissionType, ACLFilter, ACL, ResourcePattern, ResourceType, \
+    ACLResourcePatternType
 from kafka.client_async import KafkaClient, selectors
+from kafka.coordinator.protocol import ConsumerProtocolMemberMetadata, ConsumerProtocolMemberAssignment, ConsumerProtocol
 import kafka.errors as Errors
 from kafka.errors import (
     IncompatibleBrokerVersion, KafkaConfigurationError, NotControllerError,
@@ -17,13 +19,11 @@
 from kafka.protocol.admin import (
     CreateTopicsRequest, DeleteTopicsRequest, DescribeConfigsRequest, AlterConfigsRequest, CreatePartitionsRequest,
     ListGroupsRequest, DescribeGroupsRequest, DescribeAclsRequest, CreateAclsRequest, DeleteAclsRequest)
-from kafka.protocol.types import Array
-from kafka.coordinator.protocol import ConsumerProtocolMemberMetadata, ConsumerProtocolMemberAssignment, ConsumerProtocol
 from kafka.protocol.commit import GroupCoordinatorRequest, OffsetFetchRequest
 from kafka.protocol.metadata import MetadataRequest
+from kafka.protocol.types import Array
 from kafka.structs import TopicPartition, OffsetAndMetadata, MemberInformation, GroupInformation
-from kafka.admin.acl_resource import ACLOperation, ACLPermissionType, ACLFilter, ACL, ResourcePattern, ResourceType, \
-    ACLResourcePatternType
+from kafka.vendor import six
 from kafka.version import __version__
 
 
@@ -1003,39 +1003,39 @@ def _describe_consumer_groups_process_response(self, response):
         if response.API_VERSION <= 3:
             assert len(response.groups) == 1
             for response_field, response_name in zip(response.SCHEMA.fields, response.SCHEMA.names):
-                if type(response_field) == Array:
+                if isinstance(response_field, Array):
                     described_groups = response.__dict__[response_name]
                     described_groups_field_schema = response_field.array_of
-                    for described_group  in described_groups:
-                        described_group_information_list = []
-                        is_consumer_protocol_type = False
-                        for (described_group_information, group_information_name, group_information_field) in zip(described_group, described_groups_field_schema.names, described_groups_field_schema.fields):
-                            if group_information_name == 'protocol_type':
-                                protocol_type = described_group_information
-                                is_consumer_protocol_type = (protocol_type == ConsumerProtocol.PROTOCOL_TYPE or not protocol_type)
-                            if type(group_information_field) == Array:
-                                member_information_list = []
-                                member_schema = group_information_field.array_of
-                                for members in described_group_information:
-                                    member_information = []
-                                    for (member, member_field, member_name)  in zip(members, member_schema.fields, member_schema.names):
-                                        if member_name == 'member_metadata' and is_consumer_protocol_type:
+                    described_group = response.__dict__[response_name][0]
+                    described_group_information_list = []
+                    protocol_type_is_consumer = False
+                    for (described_group_information, group_information_name, group_information_field) in zip(described_group, described_groups_field_schema.names, described_groups_field_schema.fields):
+                        if group_information_name == 'protocol_type':
+                            protocol_type = described_group_information
+                            protocol_type_is_consumer = (protocol_type == ConsumerProtocol.PROTOCOL_TYPE or not protocol_type)
+                        if type(group_information_field) == Array:
+                            member_information_list = []
+                            member_schema = group_information_field.array_of
+                            for members in described_group_information:
+                                member_information = []
+                                for (member, member_field, member_name)  in zip(members, member_schema.fields, member_schema.names):
+                                    if protocol_type_is_consumer:
+                                        if member_name == 'member_metadata' and member:
                                             member_information.append(ConsumerProtocolMemberMetadata.decode(member))
-                                        elif member_name == 'member_assignment' and is_consumer_protocol_type:
+                                        elif member_name == 'member_assignment' and member:
                                             member_information.append(ConsumerProtocolMemberAssignment.decode(member))
                                         else:
                                             member_information.append(member)
-                                    else:
-                                        member_info_tuple = MemberInformation._make(member_information)
-                                        member_information_list.append(member_info_tuple)
-                                else:
-                                    described_group_information_list.append(member_information_list)
-                            else:
-                                described_group_information_list.append(described_group_information)
+                                member_info_tuple = MemberInformation._make(member_information)
+                                member_information_list.append(member_info_tuple)
+                            described_group_information_list.append(member_information_list)
                         else:
-                            if response.API_VERSION <=2:
-                                described_group_information_list.append([])
-                            group_description = GroupInformation._make(described_group_information_list)
+                            described_group_information_list.append(described_group_information)
+                    # Version 3 of the DescribeGroups API introduced the "authorized_operations" field. This will cause the namedtuple to fail
+                    # Therefore, appending a placeholder of None in it.
+                    if response.API_VERSION <=2:
+                        described_group_information_list.append(None)
+                    group_description = GroupInformation._make(described_group_information_list)
             error_code = group_description.error_code
             error_type = Errors.for_code(error_code)
             # Java has the note: KAFKA-6789, we can retry based on the error code

test/test_admin_integration.py

@@ -1,10 +1,13 @@
 import pytest
 
-from test.testutil import env_kafka_version
+from logging import info
+from test.testutil import env_kafka_version, random_string
+from threading import Event, Thread
+from time import time, sleep
 
-from kafka.errors import (NoError, GroupCoordinatorNotAvailableError)
 from kafka.admin import (
     ACLFilter, ACLOperation, ACLPermissionType, ResourcePattern, ResourceType, ACL, ConfigResource, ConfigResourceType)
+from kafka.errors import (NoError, GroupCoordinatorNotAvailableError)
 
 
 @pytest.mark.skipif(env_kafka_version() < (0, 11), reason="ACL features require broker >=0.11")
@@ -149,9 +152,88 @@ def test_describe_consumer_group_does_not_exist(kafka_admin_client):
 @pytest.mark.skipif(env_kafka_version() < (0, 11), reason='Describe consumer group requires broker >=0.11')
 def test_describe_consumer_group_exists(kafka_admin_client, kafka_consumer_factory, topic):
     """Tests that the describe consumer group call returns valid consumer group information
+    This test takes inspiration from the test 'test_group' in test_consumer_group.py.
     """
-    consumer = kafka_consumer_factory(group_id='testgrp', auto_offset_reset='earliest')
-    consumer.poll(timeout_ms=20)
-    output = kafka_admin_client.describe_consumer_groups(['testgrp'])
-    assert output[0].group == 'testgrp'
-    assert output[0].members[0].member_metadata.subscription[0] == topic
+    consumers = {}
+    stop = {}
+    threads = {}
+    random_group_id = 'test-group-' + random_string(6)
+    group_id_list = [random_group_id, random_group_id + '_2']
+    generations = {group_id_list[0]: set(), group_id_list[1]: set()}
+    def consumer_thread(i, group_id):
+        assert i not in consumers
+        assert i not in stop
+        stop[i] = Event()
+        consumers[i] = kafka_consumer_factory(group_id=group_id)
+        while not stop[i].is_set():
+            consumers[i].poll(20)
+        consumers[i].close()
+        consumers[i] = None
+        stop[i] = None
+
+    num_consumers = 3
+    for i in range(num_consumers):
+        group_id = group_id_list[i % 2]
+        t = Thread(target=consumer_thread, args=(i, group_id,))
+        t.start()
+        threads[i] = t
+
+    try:
+        timeout = time() + 35
+        while True:
+            for c in range(num_consumers):
+
+                # Verify all consumers have been created
+                if c not in consumers:
+                    break
+
+                # Verify all consumers have an assignment
+                elif not consumers[c].assignment():
+                    break
+
+            # If all consumers exist and have an assignment
+            else:
+
+                info('All consumers have assignment... checking for stable group')
+                # Verify all consumers are in the same generation
+                # then log state and break while loop
+
+                for consumer in consumers.values():
+                    generations[consumer.config['group_id']].add(consumer._coordinator._generation.generation_id)
+
+                is_same_generation = any([len(consumer_generation) == 1 for consumer_generation in generations.values()])
+
+                # New generation assignment is not complete until
+                # coordinator.rejoining = False
+                rejoining = any([consumer._coordinator.rejoining
+                                 for consumer in list(consumers.values())])
+
+                if not rejoining and is_same_generation:
+                    break
+                else:
+                    sleep(1)
+            assert time() < timeout, "timeout waiting for assignments"
+
+        info('Group stabilized; verifying assignment')
+        output = kafka_admin_client.describe_consumer_groups(group_id_list)
+        assert len(output) == 2
+        consumer_groups = set()
+        for consumer_group in output:
+            assert(consumer_group.group in group_id_list)
+            if consumer_group.group == group_id_list[0]:
+                assert(len(consumer_group.members) == 2)
+            else:
+                assert(len(consumer_group.members) == 1)
+            for member in consumer_group.members:
+                    assert(member.member_metadata.subscription[0] == topic)
+                    assert(member.member_assignment.assignment[0][0] == topic)
+            assert consumer_group.members
+            consumer_groups.add(consumer_group.group)
+        assert(sorted(list(consumer_groups)) == group_id_list)
+    finally:
+        info('Shutting down %s consumers', num_consumers)
+        for c in range(num_consumers):
+            info('Stopping consumer %s', c)
+            stop[c].set()
+            threads[c].join()
+            threads[c] = None