Return correct error code on create-producer/consumer is not complete yet #177
Conversation
|
Looks good, nice catch! Can you also verify the return code in the mocked unit test for the producer/subscribe timeout? Also, probably this should go on 1.16 release, right? |
| @@ -270,7 +270,8 @@ protected void handleSubscribe(final CommandSubscribe subscribe) { | |||
| // creation request either complete or fails. | |||
| log.warn("[{}][{}][{}] Consumer is already present on the connection", remoteAddress, topicName, | |||
| subscriptionName); | |||
| ctx.writeAndFlush(Commands.newError(requestId, ServerError.UnknownError, | |||
| ServerError error = !existingConsumerFuture.isDone() ? ServerError.ServiceNotReady : ServerError.UnknownError; | |||
| ctx.writeAndFlush(Commands.newError(requestId, error, | |||
There was a problem hiding this comment.
The idea is not not to return unknown error to client. Isn't it consumer already exists scenario?
There was a problem hiding this comment.
yes, if request is already in process then now, we are returning ServiceNotReady error instead unknown-error.
if consumer times-out before broker completes the request and if broker receives duplicate request while it is still processing the request then it was returning unknown error which was preventing client to retry again.
however, now, broker is returning ServiceNotReady error which is retriable error and client can retry later on and reconnect again.
There was a problem hiding this comment.
I guess @saandrews point is that there is still one case in which broker can return UnknownError. Though, after this fix, I think it shouldn't be possible to happen, barring any logical bug
There was a problem hiding this comment.
yes, in case of failure of existingProducerFuture, we are returning Unknown error code. In this case, should we return error-message of the exception with unknown error-code?
| log.warn("[{}][{}] Producer is already present on the connection", remoteAddress, topicName); | ||
| ctx.writeAndFlush(Commands.newError(requestId, ServerError.UnknownError, | ||
| ctx.writeAndFlush(Commands.newError(requestId, error, |
There was a problem hiding this comment.
Producer already present error?
There was a problem hiding this comment.
This is: producer-creation is still in process usecase. therefore, returning ServiceNotReady is a retriable error at client side and it can be reconnected later on.
|
@merlimat added the testcases for this error-code scenario. |
| private <T> ServerError getErrorCode(CompletableFuture<T> future) { | ||
| ServerError error = ServerError.UnknownError; | ||
| try { | ||
| future.get(0, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
can use future.getNow(null) here
* Move SimpleSerDe to api * Renamed SimpleSerDe to DefaultSerDe
Motivation
When broker receives multiple create_producer request with same producer-id before it completes producer-creation, it returns
UnknownErrorwhich misleads client while retrying the request.Modifications
if client times out before broker successfully creates producer and if client retries to create producer then broker should return
ServiceNotReadyinsteadUnknownError(if broker is still creating the producer) so, client can again retry on this recoverable error.Result
Broker returns
ServiceNotReadyerror if it producer/consumer creation is not complete so, client can retry on this known error-code.