Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

Refactor writeAndFlushResponseToClient #465

Conversation

BewareMyPower
Copy link
Collaborator

Motivation

#429 introduced a request timeout to avoid some requests being blocked by the previous requests. However, it changes the behavior of writeAndFlushResponseToClient.

Before #429, writeAndFlushResponseToClient removed all completed response futures in the head of responseQueue and then sent the completed response to client.

After #429, writeAndFlushResponseToClient removed all response futures, and use CompletableFuture#get(long timeout, TimeUnit unit) to wait until all futures are completed or expired.

We shouldn't wait any CompletableFuture in a CompletableFuture's callback.

Modifications

In writeAndFlushResponseToClient, only remove completed or expired response futures. Here we combine peek and boolean remove(Object o) for thread safety.

For responses that are expired or completed exceptionally, just skip them and don't send any response to client because Kafka client has a retry mechanism. When Kafka processes an expired request, it also doesn't send any response to client.

For responses that are completed normally, send the response and release the internal Netty buffers if necessary.

@BewareMyPower
Copy link
Collaborator Author

@dockerzhang @wuzhanpeng PTAL

@BewareMyPower BewareMyPower changed the title Refactor writeAndFlushResponseToClient [WIP] Refactor writeAndFlushResponseToClient May 5, 2021
@BewareMyPower BewareMyPower changed the title [WIP] Refactor writeAndFlushResponseToClient Refactor writeAndFlushResponseToClient May 5, 2021
@wuzhanpeng
Copy link
Contributor

LGTM 👍

@jiazhai jiazhai merged commit 3a31730 into streamnative:master May 7, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/refactor-write-and-flush branch May 8, 2021 08:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants