Author’s note: The two new methods in this file were moved here from ConsumerNetworkClient as they will be used in other classes in future commits.

Author’s note: this method is used in the new Consumer’s constructor in a future commit.

Author’s note: adding toString() implementations is helpful in debugging.

Author’s note: adding toString() implementations is helpful in debugging.

Author’s note: formatting and added debug

Author’s note: this is the logic from the now-removed requestBackoffExpired method with a log line for debugging.

Author’s note: requestBackoffExpired is now inline in canSendRequest above.

Author’s note: added a LogContext argument and formatting.

Author’s note: added a LogContext argument and formatting.

Author’s note: mostly encapsulating the instance variables for protection against accidental outside changes.

Author’s note: As stated previously, the core logic from isUnavailable and maybeThrowAuthFailure were moved to NetworkClientUtils for reuse.

Author’s note: most of these changes are related to the encapsulation changes that were made in CompletedFetch.

Author’s note: requestMetadataUpdate was moved to FetchUtils as it will be reused elsewhere in future commits.

Author’s note: renamed subscriptionState to subscriptions for consistency with the rest of the newly refactored code base.

i actually wonder if the debug would be useful due to lack of the information of what request it is. maybe this log should stay at the request manager level?

Hi @philipnee!

That's a good point. This debug logging doesn't have context of the request.

The request manager calls canSendRequest() before creating the request, though. So in the cases where we return false, there won't be a request, so there isn't any additional request information that the request manager would be able to log.

Additionally, there are two different reasons why a request cannot be sent:

1. There's already an inflight request
2. The backoff hasn't expired

When canSendRequest() returns false back to the request manager, we'd lose the context to explain the reason why the request can't be sent.

I'll look into this a bit more to see what I can do.

Thanks!

I added a relevant toString() method to the RequestState sub-classes that includes details about the request. The logging now includes the toString()-ed RequestState for context.

ditto ^^

Instead of duplicating the code in RequestState, could we pull out the common part as a util and reuse?

@junrao Are you referring to the toString() code specifically, or something else?

I can make an internal toStringBase() to RequestState that has the core instance variables concatenated as a string. And then subclasses can first append their own instance variable string and then append the result of toStringBase() or something. Does that make sense?

Yes, I was referring to the toString method. It seems that every subclass of RequestState directly gets every field in RequestState to build its own string. This creates duplicated code and can be a bit error prone.

@junrao I created toStringBase() in RequestState:

/**
 * This method appends the instance variables together in a simple String of comma-separated key value pairs.
 * This allows subclasses to include these values and not have to duplicate each variable, helping to prevent
 * any variables from being omitted when new ones are added.
 *
 * @return String version of instance variables.
 */
protected String toStringBase() {
    return "owner='" + owner + '\'' +
            ", exponentialBackoff=" + exponentialBackoff +
            ", lastSentMs=" + lastSentMs +
            ", lastReceivedMs=" + lastReceivedMs +
            ", numAttempts=" + numAttempts +
            ", backoffMs=" + backoffMs;
}

@Override
public String toString() {
    return "RequestState{" + toStringBase() + '}';
}

Subclasses look like this:

@Override
public String toString() {
    return "OffsetFetchRequestState{" +
            "requestedPartitions=" + requestedPartitions +
            ", requestedGeneration=" + requestedGeneration +
            ", future=" + future +
            ", " + toStringBase() +
            '}';
}

Does that seem OK?

I assume this visibility change is needed for a subsequent commit?

Yes.