frankvicky commented on code in PR #17549:
URL: https://github.com/apache/kafka/pull/17549#discussion_r1810581161
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##########
@@ -1351,6 +1355,7 @@ private void throwIfShareGroupHeartbeatRequestIsInvalid(
if (request.subscribedTopicNames() == null ||
request.subscribedTopicNames().isEmpty()) {
throw new InvalidRequestException("SubscribedTopicNames must
be set in first request.");
}
+ throwIfEmptyString(request.memberId(), "MemberId can't be empty.");
Review Comment:
Since consumers are now required to generate their own `memberId` and the
`ShareGroupHeartbeat RPC` is still under development, it seems feasible to
check the `memberId` at the beginning:
```java
private void throwIfShareGroupHeartbeatRequestIsInvalid(
ShareGroupHeartbeatRequestData request
) throws InvalidRequestException, UnsupportedAssignorException {
throwIfEmptyString(request.groupId(), "GroupId can't be empty.");
throwIfEmptyString(request.rackId(), "RackId can't be empty.");
throwIfEmptyString(request.memberId(), "MemberId can't be empty."); //
Always check memberId
if (request.memberEpoch() == 0) {
if (request.subscribedTopicNames() == null ||
request.subscribedTopicNames().isEmpty()) {
throw new InvalidRequestException("SubscribedTopicNames must be
set in first request.");
}
} else if (request.memberEpoch() <
ShareGroupHeartbeatRequest.LEAVE_GROUP_MEMBER_EPOCH) {
throw new InvalidRequestException("MemberEpoch is invalid.");
}
}
```
Dose this make sense ?
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java:
##########
@@ -1351,6 +1355,7 @@ private void throwIfShareGroupHeartbeatRequestIsInvalid(
if (request.subscribedTopicNames() == null ||
request.subscribedTopicNames().isEmpty()) {
throw new InvalidRequestException("SubscribedTopicNames must
be set in first request.");
}
+ throwIfEmptyString(request.memberId(), "MemberId can't be empty.");
Review Comment:
Since consumers are now required to generate their own `memberId` and the
`ShareGroupHeartbeat RPC` is still under development, it seems feasible to
check the `memberId` at the beginning:
```java
private void throwIfShareGroupHeartbeatRequestIsInvalid(
ShareGroupHeartbeatRequestData request
) throws InvalidRequestException, UnsupportedAssignorException {
throwIfEmptyString(request.groupId(), "GroupId can't be empty.");
throwIfEmptyString(request.rackId(), "RackId can't be empty.");
throwIfEmptyString(request.memberId(), "MemberId can't be empty."); //
Always check memberId
if (request.memberEpoch() == 0) {
if (request.subscribedTopicNames() == null ||
request.subscribedTopicNames().isEmpty()) {
throw new InvalidRequestException("SubscribedTopicNames must be
set in first request.");
}
} else if (request.memberEpoch() <
ShareGroupHeartbeatRequest.LEAVE_GROUP_MEMBER_EPOCH) {
throw new InvalidRequestException("MemberEpoch is invalid.");
}
}
```
Dose this make sense ?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
