Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -568,10 +568,14 @@ RequestFuture<ByteBuffer> sendJoinGroupRequest() {

int joinGroupTimeoutMs = Math.max(rebalanceConfig.rebalanceTimeoutMs, rebalanceConfig.rebalanceTimeoutMs + 5000);
return client.send(coordinator, requestBuilder, joinGroupTimeoutMs)
.compose(new JoinGroupResponseHandler());
.compose(new JoinGroupResponseHandler(generation));
}

private class JoinGroupResponseHandler extends CoordinatorResponseHandler<JoinGroupResponse, ByteBuffer> {
private JoinGroupResponseHandler(final Generation generation) {
super(generation);
}

@Override
public void handle(JoinGroupResponse joinResponse, RequestFuture<ByteBuffer> future) {
Errors error = joinResponse.error();
Expand Down Expand Up @@ -606,9 +610,12 @@ public void handle(JoinGroupResponse joinResponse, RequestFuture<ByteBuffer> fut
// backoff and retry
future.raise(error);
} else if (error == Errors.UNKNOWN_MEMBER_ID) {
// reset the member id and retry immediately
resetGenerationOnResponseError(ApiKeys.JOIN_GROUP, error);
log.debug("Attempt to join group failed due to unknown member id.");
log.debug("Attempt to join group failed due to unknown member id with {}.", sentGeneration);
// only need to reset the member id if generation has not been changed,
// then retry immediately
if (generationUnchanged())
resetGenerationOnResponseError(ApiKeys.JOIN_GROUP, error);

future.raise(error);
} else if (error == Errors.COORDINATOR_NOT_AVAILABLE
|| error == Errors.NOT_COORDINATOR) {
Expand All @@ -617,7 +624,10 @@ public void handle(JoinGroupResponse joinResponse, RequestFuture<ByteBuffer> fut
log.debug("Attempt to join group failed due to obsolete coordinator information: {}", error.message());
future.raise(error);
} else if (error == Errors.FENCED_INSTANCE_ID) {
log.error("Received fatal exception: group.instance.id gets fenced");
// for join-group request, even if the generation has changed we would not expect the instance id
// gets fenced, and hence we always treat this as a fatal error
log.error("Attempt to join group with generation {} failed because the group instance id {} has been fenced by another instance",
rebalanceConfig.groupInstanceId, sentGeneration);
future.raise(error);
} else if (error == Errors.INCONSISTENT_GROUP_PROTOCOL
|| error == Errors.INVALID_SESSION_TIMEOUT
Expand Down Expand Up @@ -708,10 +718,14 @@ private RequestFuture<ByteBuffer> sendSyncGroupRequest(SyncGroupRequest.Builder
if (coordinatorUnknown())
return RequestFuture.coordinatorNotAvailable();
return client.send(coordinator, requestBuilder)
.compose(new SyncGroupResponseHandler());
.compose(new SyncGroupResponseHandler(generation));
}

private class SyncGroupResponseHandler extends CoordinatorResponseHandler<SyncGroupResponse, ByteBuffer> {
private SyncGroupResponseHandler(final Generation generation) {
super(generation);
}

@Override
public void handle(SyncGroupResponse syncResponse,
RequestFuture<ByteBuffer> future) {
Expand All @@ -737,16 +751,21 @@ public void handle(SyncGroupResponse syncResponse,
log.debug("SyncGroup failed because the group began another rebalance");
future.raise(error);
} else if (error == Errors.FENCED_INSTANCE_ID) {
log.error("Received fatal exception: group.instance.id gets fenced");
// for sync-group request, even if the generation has changed we would not expect the instance id
// gets fenced, and hence we always treat this as a fatal error
log.error("SyncGroup with {} failed because the group instance id {} has been fenced by another instance",
sentGeneration, rebalanceConfig.groupInstanceId);
future.raise(error);
} else if (error == Errors.UNKNOWN_MEMBER_ID
|| error == Errors.ILLEGAL_GENERATION) {
log.debug("SyncGroup failed: {}", error.message());
resetGenerationOnResponseError(ApiKeys.SYNC_GROUP, error);
log.info("SyncGroup with {} failed: {}, would request re-join", sentGeneration, error.message());
if (generationUnchanged())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @guozhangwang.
Trying to understand if any of these changes have implications for the WorkerCoordinator too.

What I've observed is that if the WorkerCoordinator fails to receive a valid SyncGroup request, it may miss several generations until it succeeds again. Is the branch here safe? resetGenerationOnResponseError will be called (and therefore rejoin will be requested) only if generationUnchanged returns true. Is there any risk that we suppress any useful retries to rejoin?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rationale is that if the generation has changed, it is either reset by the heartbeat thread in which case the generation is reset and rejoin is already requested, or it is changed by another join-group request; but since inside AbstractCoordinator we will only have one in-flight request at a given time the second scenario should not happen. So the only possibility is the heartbeat resetting.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks

resetGenerationOnResponseError(ApiKeys.SYNC_GROUP, error);

future.raise(error);
} else if (error == Errors.COORDINATOR_NOT_AVAILABLE
|| error == Errors.NOT_COORDINATOR) {
log.debug("SyncGroup failed: {}", error.message());
log.debug("SyncGroup failed: {}, marking coordinator unknown", error.message());
markCoordinatorUnknown();
future.raise(error);
} else {
Expand Down Expand Up @@ -890,8 +909,8 @@ protected synchronized String memberId() {
}

private synchronized void resetGeneration() {
this.rejoinNeeded = true;
this.generation = Generation.NO_GENERATION;
rejoinNeeded = true;
generation = Generation.NO_GENERATION;
}

synchronized void resetGenerationOnResponseError(ApiKeys api, Errors error) {
Expand Down Expand Up @@ -989,7 +1008,7 @@ public synchronized RequestFuture<Void> maybeLeaveGroup(String leaveReason) {
Collections.singletonList(new MemberIdentity().setMemberId(generation.memberId))
);

future = client.send(coordinator, request).compose(new LeaveGroupResponseHandler());
future = client.send(coordinator, request).compose(new LeaveGroupResponseHandler(generation));
client.pollNoWakeup();
}

Expand All @@ -1003,6 +1022,10 @@ protected boolean isDynamicMember() {
}

private class LeaveGroupResponseHandler extends CoordinatorResponseHandler<LeaveGroupResponse, Void> {
private LeaveGroupResponseHandler(final Generation generation) {
super(generation);
}

@Override
public void handle(LeaveGroupResponse leaveResponse, RequestFuture<Void> future) {
final List<MemberResponse> members = leaveResponse.memberResponses();
Expand All @@ -1013,10 +1036,10 @@ public void handle(LeaveGroupResponse leaveResponse, RequestFuture<Void> future)

final Errors error = leaveResponse.error();
if (error == Errors.NONE) {
log.debug("LeaveGroup request returned successfully");
log.debug("LeaveGroup response with {} returned successfully: {}", sentGeneration, response);
future.complete(null);
} else {
log.error("LeaveGroup request failed with error: {}", error.message());
log.error("LeaveGroup request with {} failed with error: {}", sentGeneration, error.message());
future.raise(error);
}
}
Expand All @@ -1037,10 +1060,8 @@ synchronized RequestFuture<Void> sendHeartbeatRequest() {
}

private class HeartbeatResponseHandler extends CoordinatorResponseHandler<HeartbeatResponse, Void> {
private final Generation sentGeneration;

private HeartbeatResponseHandler(final Generation generation) {
this.sentGeneration = generation;
super(generation);
}

@Override
Expand All @@ -1060,17 +1081,20 @@ public void handle(HeartbeatResponse heartbeatResponse, RequestFuture<Void> futu
log.info("Attempt to heartbeat failed since group is rebalancing");
requestRejoin();
future.raise(error);
} else if (error == Errors.ILLEGAL_GENERATION) {
log.info("Attempt to heartbeat failed since generation {} is not current", sentGeneration.generationId);
resetGenerationOnResponseError(ApiKeys.HEARTBEAT, error);
future.raise(error);
} else if (error == Errors.FENCED_INSTANCE_ID) {
log.error("Received fatal exception: group.instance.id gets fenced");
future.raise(error);
} else if (error == Errors.UNKNOWN_MEMBER_ID) {
log.info("Attempt to heartbeat failed since member id {} is not valid.", sentGeneration.memberId);
resetGenerationOnResponseError(ApiKeys.HEARTBEAT, error);
future.raise(error);
} else if (error == Errors.ILLEGAL_GENERATION ||
error == Errors.UNKNOWN_MEMBER_ID ||
error == Errors.FENCED_INSTANCE_ID) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the consumer has been legitimately fenced, is it safe to rejoin the group after resetting the member id? Would that not lead to a ping-pong scenario?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note since we set the InstanceFenced error in future, and hence in the caller:

final RuntimeException exception = future.exception();
                log.info("Join group failed with {}", exception.toString());
                resetJoinGroupFuture();
                if (exception instanceof UnknownMemberIdException ||
                    exception instanceof RebalanceInProgressException ||
                    exception instanceof IllegalGenerationException ||
                    exception instanceof MemberIdRequiredException)
                    continue;
                else if (!future.isRetriable())
                    throw exception;

We would throw that retriable exception still.

if (generationUnchanged()) {
log.info("Attempt to heartbeat with {} and group instance id {} failed due to {}, resetting generation",
sentGeneration, rebalanceConfig.groupInstanceId, error);
resetGenerationOnResponseError(ApiKeys.HEARTBEAT, error);
future.raise(error);
} else {
// if the generation has changed, then ignore this error
log.info("Attempt to heartbeat with stale {} and group instance id {} failed due to {}, ignoring the error",
sentGeneration, rebalanceConfig.groupInstanceId, error);
future.complete(null);
}
} else if (error == Errors.GROUP_AUTHORIZATION_FAILED) {
future.raise(GroupAuthorizationException.forGroupId(rebalanceConfig.groupId));
} else {
Expand All @@ -1080,7 +1104,12 @@ public void handle(HeartbeatResponse heartbeatResponse, RequestFuture<Void> futu
}

protected abstract class CoordinatorResponseHandler<R, T> extends RequestFutureAdapter<ClientResponse, T> {
protected ClientResponse response;
CoordinatorResponseHandler(final Generation generation) {
this.sentGeneration = generation;
}

final Generation sentGeneration;
ClientResponse response;

public abstract void handle(R response, RequestFuture<T> future);

Expand All @@ -1106,6 +1135,11 @@ public void onSuccess(ClientResponse clientResponse, RequestFuture<T> future) {
}
}

boolean generationUnchanged() {
synchronized (AbstractCoordinator.this) {
return generation.equals(sentGeneration);
}
}
}

protected Meter createMeter(Metrics metrics, String groupName, String baseName, String descriptiveName) {
Expand Down Expand Up @@ -1421,11 +1455,11 @@ private static class UnjoinedGroupException extends RetriableException {
}

// For testing only below
public Heartbeat heartbeat() {
final Heartbeat heartbeat() {
return heartbeat;
}

final void setLastRebalanceTime(final long timestamp) {
final synchronized void setLastRebalanceTime(final long timestamp) {
lastRebalanceEndMs = timestamp;
}

Expand All @@ -1449,4 +1483,12 @@ final boolean hasUnknownGeneration() {
final boolean hasValidMemberId() {
return generation != Generation.NO_GENERATION && generation.hasMemberId();
}

final synchronized void setNewGeneration(final Generation generation) {
this.generation = generation;
}

final synchronized void setNewState(final MemberState state) {
this.state = state;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,18 @@ private void maybeUpdateJoinedSubscription(Set<TopicPartition> assignedPartition
}
}

private Exception invokeOnAssignment(final ConsumerPartitionAssignor assignor, final Assignment assignment) {
log.info("Notifying assignor about the new {}", assignment);

try {
assignor.onAssignment(assignment, groupMetadata);
} catch (Exception e) {
return e;
}

return null;
}

private Exception invokePartitionsAssigned(final Set<TopicPartition> assignedPartitions) {
log.info("Adding newly assigned partitions: {}", Utils.join(assignedPartitions, ", "));

Expand Down Expand Up @@ -351,7 +363,7 @@ protected void onJoinComplete(int generation,

// should at least encode the short version
if (assignmentBuffer.remaining() < 2)
throw new IllegalStateException("There is insufficient bytes available to read assignment from the sync-group response (" +
throw new IllegalStateException("There are insufficient bytes available to read assignment from the sync-group response (" +
"actual byte size " + assignmentBuffer.remaining() + ") , this is not expected; " +
"it is possible that the leader's assign function is buggy and did not return any assignment for this member, " +
"or because static member is configured and the protocol is buggy hence did not get the assignment for this member");
Expand Down Expand Up @@ -406,11 +418,7 @@ protected void onJoinComplete(int generation,
maybeUpdateJoinedSubscription(assignedPartitions);

// Catch any exception here to make sure we could complete the user callback.
try {
assignor.onAssignment(assignment, groupMetadata);
} catch (Exception e) {
firstException.compareAndSet(null, e);
}
firstException.compareAndSet(null, invokeOnAssignment(assignor, assignment));

// Reschedule the auto commit starting from now
if (autoCommitEnabled)
Expand Down Expand Up @@ -1064,10 +1072,12 @@ public void onComplete(Map<TopicPartition, OffsetAndMetadata> offsets, Exception
* which returns a request future that can be polled in the case of a synchronous commit or ignored in the
* asynchronous case.
*
* NOTE: This is visible only for testing
*
* @param offsets The list of offsets per partition that should be committed.
* @return A request future whose value indicates whether the commit was successful or not
*/
private RequestFuture<Void> sendOffsetCommitRequest(final Map<TopicPartition, OffsetAndMetadata> offsets) {
RequestFuture<Void> sendOffsetCommitRequest(final Map<TopicPartition, OffsetAndMetadata> offsets) {
if (offsets.isEmpty())
return RequestFuture.voidSuccess();

Expand Down Expand Up @@ -1135,14 +1145,14 @@ private RequestFuture<Void> sendOffsetCommitRequest(final Map<TopicPartition, Of
log.trace("Sending OffsetCommit request with {} to coordinator {}", offsets, coordinator);

return client.send(coordinator, builder)
.compose(new OffsetCommitResponseHandler(offsets));
.compose(new OffsetCommitResponseHandler(offsets, generation));
Comment thread
hachikuji marked this conversation as resolved.
}

private class OffsetCommitResponseHandler extends CoordinatorResponseHandler<OffsetCommitResponse, Void> {

private final Map<TopicPartition, OffsetAndMetadata> offsets;

private OffsetCommitResponseHandler(Map<TopicPartition, OffsetAndMetadata> offsets) {
private OffsetCommitResponseHandler(Map<TopicPartition, OffsetAndMetadata> offsets, Generation generation) {
super(generation);
this.offsets = offsets;
}

Expand Down Expand Up @@ -1190,8 +1200,16 @@ public void handle(OffsetCommitResponse commitResponse, RequestFuture<Void> futu
future.raise(error);
return;
} else if (error == Errors.FENCED_INSTANCE_ID) {
log.error("Received fatal exception: group.instance.id gets fenced");
future.raise(error);
log.info("OffsetCommit failed with {} due to group instance id {} fenced", sentGeneration, rebalanceConfig.groupInstanceId);

// if the generation has changed, do not raise the fatal error but rebalance-in-progress
if (generationUnchanged()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why could we still survive from a fenced instance id in commit request?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we could not "survive", the point of throwing RebalanceInProgress is not to survive, but to give a different error case to the caller that it do not necessarily have to exit as zombie but could retry.

future.raise(error);
} else {
future.raise(new RebalanceInProgressException("Offset commit cannot be completed since the " +
"consumer member's old generation is fenced by its group instance id, it is possible that " +
"this consumer has already participated another rebalance and got a new generation"));
}
return;
} else if (error == Errors.REBALANCE_IN_PROGRESS) {
/* Consumer should not try to commit offset in between join-group and sync-group,
Expand All @@ -1209,9 +1227,18 @@ public void handle(OffsetCommitResponse commitResponse, RequestFuture<Void> futu
return;
} else if (error == Errors.UNKNOWN_MEMBER_ID
|| error == Errors.ILLEGAL_GENERATION) {
// need to reset generation and re-join group
resetGenerationOnResponseError(ApiKeys.OFFSET_COMMIT, error);
future.raise(new CommitFailedException());
log.info("OffsetCommit failed with {}: {}", sentGeneration, error.message());

// only need to reset generation and re-join group if generation has not changed;
// otherwise only raise rebalance-in-progress error
if (generationUnchanged()) {
resetGenerationOnResponseError(ApiKeys.OFFSET_COMMIT, error);
future.raise(new CommitFailedException());
} else {
future.raise(new RebalanceInProgressException("Offset commit cannot be completed since the " +

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit misleading, because the consumer is not actually participating in an ongoing rebalance (yet)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guozhangwang can you elaborate on why we don't just throw CommitFailedException here? It seems like it must be the case that the caller is a zombie and should get that exception to alert it, not the RebalanceInProgressException which seems to indicate it might be recoverable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the generation has changed since the commit request is sent, then it is likely that it has participated in a new rebalance (and hence get a new generation), or it has reset its generation due to the heartbeat failure. So this commit failure is recoverable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted about this offline, will just leave the concrete proposal here:

...
} else {
    if (state == MemberState.REBALANCING) {
        future.raise(new RebalanceInProgressException(...)
    } else {
        future.raise(new CommitFailedException(...)
    }
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning being that we use CommitFailedException to signal we have dropped out of the group, and RebalanceInProgressException to signal that a rebalance is in progress. There are two cases to consider (within the general case of the generation having changed):

  1. If the generation is unknown and the state is STABLE this means we have dropped out of the group, but haven't yet rejoined and haven't invoked onPartitionsLost --> should throw CommitFailed
  2. If the generation is unknown and the state is REBALANCING this means we dropped out of the group, but have already noticed and rejoined, and already invoked onPartitionsLost --> should throw RebalanceInProgress

Note that if we dropped out of the group and already completed the rejoin, the state will be STABLE again but the generation will also have been set so this case does not apply. Basically, we want to keep CommitFailedException to indicate that the consumer definitely dropped out and will have to rejoin, which is the case in 1. above

"consumer member's generation is already stale, meaning it has already participated another rebalance and " +
"got a new generation. You can try completing the rebalance by calling poll() and then retry commit again"));
}
return;
} else {
future.raise(new KafkaException("Unexpected error in commit: " + error.message()));
Expand Down Expand Up @@ -1253,6 +1280,10 @@ private RequestFuture<Map<TopicPartition, OffsetAndMetadata>> sendOffsetFetchReq
}

private class OffsetFetchResponseHandler extends CoordinatorResponseHandler<OffsetFetchResponse, Map<TopicPartition, OffsetAndMetadata>> {
private OffsetFetchResponseHandler() {
super(Generation.NO_GENERATION);
}

@Override
public void handle(OffsetFetchResponse response, RequestFuture<Map<TopicPartition, OffsetAndMetadata>> future) {
if (response.hasError()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,11 @@ public Map<Errors, Integer> errorCounts() {
return combinedErrorCounts;
}

@Override
public String toString() {
return data.toString();
}

@Override
public Struct toStruct(short version) {
return data.toStruct(version);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ protected Struct toStruct(short version) {
return data.toStruct(version);
}

@Override
public String toString() {
return data.toString();
}

public static SyncGroupResponse parse(ByteBuffer buffer, short version) {
return new SyncGroupResponse(ApiKeys.SYNC_GROUP.parseResponse(version, buffer), version);
}
Expand Down
Loading