Incremental fetch support in Redpanda

[mmaslankaprv]

Incremental fetch support was introduced in FetchRequest version 7. We have to add fetch session management to redpanda.

More details:
Kip-227 Incremental Fetch Requests

[dotnwat]

The KIP introduction seems to describe this as a feature of inter-broker protocol. This also affects clients? Is this blocking anything? How should we prioritize this?

[mmaslankaprv]

It is not blocking. This is also supported by the Kafka client. We can live with version 6 of fetch request for now.

[mmaslankaprv]

Problem can be easily reproduced with open messaging benchmark

[dotnwat]

What is the problem?

[mmaslankaprv]

Right now Redpanda claims that it support FetchRequest up to version 10, client expect that some of the partitions will be fetch automatically even when they are not explicitly listed in FetchRequest. When this happen, client treats it as an error.

[mmaslankaprv]

This feature is implemented in Kafka's FetchSessionHandler::hanldeResponse

[mmaslankaprv]

Here is the client part that validates incremental fetch response:

 String problem = verifyIncrementalFetchResponsePartitions(response);
            if (problem != null) {
                log.info("Node {} sent an invalid incremental fetch response with {}", node, problem);
                nextMetadata = nextMetadata.nextCloseExisting();
                return false;
            } else if (response.sessionId() == INVALID_SESSION_ID) {
                // The incremental fetch session was closed by the server.
                if (log.isDebugEnabled())
                    log.debug("Node {} sent an incremental fetch response closing session {}{}",
                            node, nextMetadata.sessionId(), responseDataToLogString(response));
                nextMetadata = FetchMetadata.INITIAL;
                return true;
            } else {
                // The incremental fetch session was continued by the server.
                // We don't have to do anything special here to support KIP-219, since an empty incremental
                // fetch request is perfectly valid.
                if (log.isDebugEnabled())
                    log.debug("Node {} sent an incremental fetch response with throttleTimeMs = {} " +
                        "for session {}{}", response.throttleTimeMs(), node, response.sessionId(),
                        responseDataToLogString(response));
                nextMetadata = nextMetadata.nextIncremental();
                return true;
            }

[dotnwat]

Thanks for the explanation of the issue, @mmaslankaprv :) Have you tried reducing the upper limit on the version of fetch that we support? Unfortunately we haven't been tracking closely which max version we can get away with, but the higher we go the more features that are in the protocol format but we don't yet support.

[mmaslankaprv]

@dotnwat there is a PR already for this in redpanda repo.

[dotnwat]

A PR that adds support for incremental fetch support?

[mmaslankaprv]

This might be helpful, it determine when to send full vs incremental Fetch response

public class FetchMetadata {
    public static final Logger log = LoggerFactory.getLogger(FetchMetadata.class);

    /**
     * The session ID used by clients with no session.
     */
    public static final int INVALID_SESSION_ID = 0;

    /**
     * The first epoch.  When used in a fetch request, indicates that the client
     * wants to create or recreate a session.
     */
    public static final int INITIAL_EPOCH = 0;

    /**
     * An invalid epoch.  When used in a fetch request, indicates that the client
     * wants to close any existing session, and not create a new one.
     */
    public static final int FINAL_EPOCH = -1;

    /**
     * The FetchMetadata that is used when initializing a new FetchSessionHandler.
     */
    public static final FetchMetadata INITIAL = new FetchMetadata(INVALID_SESSION_ID, INITIAL_EPOCH);

    /**
     * The FetchMetadata that is implicitly used for handling older FetchRequests that
     * don't include fetch metadata.
     */
    public static final FetchMetadata LEGACY = new FetchMetadata(INVALID_SESSION_ID, FINAL_EPOCH);

    /**
     * Returns the next epoch.
     *
     * @param prevEpoch The previous epoch.
     * @return          The next epoch.
     */
    public static int nextEpoch(int prevEpoch) {
        if (prevEpoch < 0) {
            // The next epoch after FINAL_EPOCH is always FINAL_EPOCH itself.
            return FINAL_EPOCH;
        } else if (prevEpoch == Integer.MAX_VALUE) {
            return 1;
        } else {
            return prevEpoch + 1;
        }
    }

    /**
     * The fetch session ID.
     */
    private final int sessionId;

    /**
     * The fetch session epoch.
     */
    private final int epoch;

    public FetchMetadata(int sessionId, int epoch) {
        this.sessionId = sessionId;
        this.epoch = epoch;
    }

    /**
     * Returns true if this is a full fetch request.
     */
    public boolean isFull() {
        return (this.epoch == INITIAL_EPOCH) || (this.epoch == FINAL_EPOCH);
    }

    public int sessionId() {
        return sessionId;
    }

    public int epoch() {
        return epoch;
    }

    @Override
    public int hashCode() {
        return Objects.hash(sessionId, epoch);
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        FetchMetadata that = (FetchMetadata) o;
        return sessionId == that.sessionId && epoch == that.epoch;
    }

    /**
     * Return the metadata for the next error response.
     */
    public FetchMetadata nextCloseExisting() {
        return new FetchMetadata(sessionId, INITIAL_EPOCH);
    }

    /**
     * Return the metadata for the next full fetch request.
     */
    public static FetchMetadata newIncremental(int sessionId) {
        return new FetchMetadata(sessionId, nextEpoch(INITIAL_EPOCH));
    }

    /**
     * Return the metadata for the next incremental response.
     */
    public FetchMetadata nextIncremental() {
        return new FetchMetadata(sessionId, nextEpoch(epoch));
    }

    @Override
    public String toString() {
        StringBuilder bld = new StringBuilder();
        if (sessionId == INVALID_SESSION_ID) {
            bld.append("(sessionId=INVALID, ");
        } else {
            bld.append("(sessionId=").append(sessionId).append(", ");
        }
        if (epoch == INITIAL_EPOCH) {
            bld.append("epoch=INITIAL)");
        } else if (epoch == FINAL_EPOCH) {
            bld.append("epoch=FINAL)");
        } else {
            bld.append("epoch=").append(epoch).append(")");
        }
        return bld.toString();
    }
}

[dotnwat]

A recap of our discussion:

• changing back to v6 to avoid incremental fetch feature means that we need to be careful about clients that might have some lower bound support.
• we are going to need to support newer versions of fetch in the near future
• incremental fetch appears to be a reasonably quick feature to add

if it is indeed fairly quick, that is strictly better than reducing the max version supported. next step is to get an accurate estimate on the cost to add incremental fetch feature.