[WIP] KAFKA-19053: Remove FetchResponse#of which is not used in production …

[apalan60]

No description provided.

[junrao]

@apalan60 : Thanks for the PR. Left a comment.

[junrao]

Could we remove the TODO for the other of constructor?

[apalan60]

@junrao

Thanks for your review.
The other of constructor is currently used by KafkaApis in production code. I'm looking into alternative approaches to remove this dependency, so that we can achieve the deletion behavior mentioned in the TODO comment. I'll update the thread if I manage to come up with a viable solution.

If I've misunderstood anything, please let me know.

[ijuma]

Are all the tests we deleted for the of method? Or are these tests for other functionality and they just happened to use of to create the fetch response?

[FrankYang0529]

The FetchResponse.of is not used in production code, but we still need to use another function to replace it in test code. Probably, you can modify this kind of function like following and add related client.prepareResponse back in the test code.

    private FetchResponse fullFetchResponse(int sessionId, TopicIdPartition tp, MemoryRecords records, Errors error, long hw,
                                            long lastStableOffset, int throttleTime) {
        return new FetchResponse(new FetchResponseData()
            .setThrottleTimeMs(throttleTime)
            .setErrorCode(Errors.NONE.code())
            .setSessionId(sessionId)
            .setResponses(List.of(
                new FetchResponseData.FetchableTopicResponse()
                    .setTopic(tp.topic())
                    .setTopicId(tp.topicId())
                    .setPartitions(List.of(
                        new FetchResponseData.PartitionData()
                            .setPartitionIndex(tp.topicPartition().partition())
                            .setErrorCode(error.code())
                            .setHighWatermark(hw)
                            .setLastStableOffset(lastStableOffset)
                            .setLogStartOffset(0)
                            .setRecords(records))
                    ))));
    }

[apalan60]

@FrankYang0529

Thanks for the review.
I really appreciate your detailed explanation and pointing out my mistake.
I’ll try out your suggested solution and update the PR accordingly.

[apalan60]

ijuma

@ijuma

Thanks for the review.
I believe I misunderstood the scope of the deletion. I'm currently working on restoring the tests that were mistakenly removed, and trying to construct the FetchResponse without using the of method, so that the original test logic is preserved while removing the dependency on the of method.

Thanks again for pointing that out. I'll update the PR soon with an improved version.