diff --git a/core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala b/core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala index 772d51d19f5c4..f178dc73fe7fe 100644 --- a/core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala +++ b/core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala @@ -145,17 +145,11 @@ class GroupCoordinator(val brokerId: Int, responseCallback(JoinGroupResult(memberId, Errors.INVALID_SESSION_TIMEOUT)) } else { val isUnknownMember = memberId == JoinGroupRequest.UNKNOWN_MEMBER_ID - groupManager.getGroup(groupId) match { + // group is created if it does not exist and the member id is UNKNOWN. if member + // is specified but group does not exist, request is rejected with UNKNOWN_MEMBER_ID + groupManager.getOrMaybeCreateGroup(groupId, isUnknownMember) match { case None => - // only try to create the group if the group is UNKNOWN AND - // the member id is UNKNOWN, if member is specified but group does not - // exist we should reject the request. - if (isUnknownMember) { - val group = groupManager.addGroup(new GroupMetadata(groupId, Empty, time)) - doUnknownJoinGroup(group, groupInstanceId, requireKnownMemberId, clientId, clientHost, rebalanceTimeoutMs, sessionTimeoutMs, protocolType, protocols, responseCallback) - } else { - responseCallback(JoinGroupResult(memberId, Errors.UNKNOWN_MEMBER_ID)) - } + responseCallback(JoinGroupResult(memberId, Errors.UNKNOWN_MEMBER_ID)) case Some(group) => group.inLock { if ((groupIsOverCapacity(group) @@ -175,9 +169,9 @@ class GroupCoordinator(val brokerId: Int, joinPurgatory.checkAndComplete(GroupKey(group.groupId)) } } - } } } + } private def doUnknownJoinGroup(group: GroupMetadata, groupInstanceId: Option[String], diff --git a/core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala b/core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala index 9f14e13435b19..230e30d079e7d 100644 --- a/core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala +++ b/core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala @@ -208,13 +208,25 @@ class GroupMetadataManager(brokerId: Int, case None => false } + /** - * Get the group associated with the given groupId, or null if not found + * Get the group associated with the given groupId or null if not found */ def getGroup(groupId: String): Option[GroupMetadata] = { Option(groupMetadataCache.get(groupId)) } + /** + * Get the group associated with the given groupId - the group is created if createIfNotExist + * is true - or null if not found + */ + def getOrMaybeCreateGroup(groupId: String, createIfNotExist: Boolean): Option[GroupMetadata] = { + if (createIfNotExist) + Option(groupMetadataCache.getAndMaybePut(groupId, new GroupMetadata(groupId, Empty, time))) + else + Option(groupMetadataCache.get(groupId)) + } + /** * Add a group or get the group associated with the given groupId if it already exists */ diff --git a/core/src/test/scala/unit/kafka/coordinator/group/GroupCoordinatorConcurrencyTest.scala b/core/src/test/scala/unit/kafka/coordinator/group/GroupCoordinatorConcurrencyTest.scala index 50f0f5e7334c3..a16aef7ea14a1 100644 --- a/core/src/test/scala/unit/kafka/coordinator/group/GroupCoordinatorConcurrencyTest.scala +++ b/core/src/test/scala/unit/kafka/coordinator/group/GroupCoordinatorConcurrencyTest.scala @@ -17,6 +17,7 @@ package kafka.coordinator.group +import java.util.Properties import java.util.concurrent.{ConcurrentHashMap, TimeUnit} import kafka.common.OffsetAndMetadata @@ -69,6 +70,8 @@ class GroupCoordinatorConcurrencyTest extends AbstractCoordinatorConcurrencyTest new LeaveGroupOperation ) + var heartbeatPurgatory: DelayedOperationPurgatory[DelayedHeartbeat] = _ + var joinPurgatory: DelayedOperationPurgatory[DelayedJoin] = _ var groupCoordinator: GroupCoordinator = _ @Before @@ -86,8 +89,8 @@ class GroupCoordinatorConcurrencyTest extends AbstractCoordinatorConcurrencyTest val config = KafkaConfig.fromProps(serverProps) - val heartbeatPurgatory = new DelayedOperationPurgatory[DelayedHeartbeat]("Heartbeat", timer, config.brokerId, reaperEnabled = false) - val joinPurgatory = new DelayedOperationPurgatory[DelayedJoin]("Rebalance", timer, config.brokerId, reaperEnabled = false) + heartbeatPurgatory = new DelayedOperationPurgatory[DelayedHeartbeat]("Heartbeat", timer, config.brokerId, reaperEnabled = false) + joinPurgatory = new DelayedOperationPurgatory[DelayedJoin]("Rebalance", timer, config.brokerId, reaperEnabled = false) groupCoordinator = GroupCoordinator(config, zkClient, replicaManager, heartbeatPurgatory, joinPurgatory, timer.time, new Metrics()) groupCoordinator.startup(false) @@ -124,6 +127,34 @@ class GroupCoordinatorConcurrencyTest extends AbstractCoordinatorConcurrencyTest verifyConcurrentRandomSequences(createGroupMembers, allOperationsWithTxn) } + @Test + def testConcurrentJoinGroupEnforceGroupMaxSize(): Unit = { + val groupMaxSize = 1 + val newProperties = new Properties + newProperties.put(KafkaConfig.GroupMaxSizeProp, groupMaxSize.toString) + val config = KafkaConfig.fromProps(serverProps, newProperties) + + if (groupCoordinator != null) + groupCoordinator.shutdown() + groupCoordinator = GroupCoordinator(config, zkClient, replicaManager, heartbeatPurgatory, + joinPurgatory, timer.time, new Metrics()) + groupCoordinator.startup(false) + + val members = new Group(s"group", nMembersPerGroup, groupCoordinator, replicaManager) + .members + val joinOp = new JoinGroupOperation() + + verifyConcurrentActions(members.toSet.map(joinOp.actionNoVerify)) + + val errors = members.map { member => + val joinGroupResult = joinOp.await(member, DefaultRebalanceTimeout) + joinGroupResult.error + } + + assertEquals(groupMaxSize, errors.count(_ == Errors.NONE)) + assertEquals(members.size-groupMaxSize, errors.count(_ == Errors.GROUP_MAX_SIZE_REACHED)) + } + abstract class GroupOperation[R, C] extends Operation { val responseFutures = new ConcurrentHashMap[GroupMember, Future[R]]()