KREST-4450 500 error when topic not present

[ehumber]

This is a user error - they've specified a topic that doesn't exist, not a server error

So the return code should be a 4xx variant, not a 5xx variant.

Tested in a running kafka-rest

< HTTP/1.1 200 OK
< Date: Thu, 14 Apr 2022 12:41:17 GMT
< Content-Type: application/json
< Vary: Accept-Encoding, User-Agent
< Transfer-Encoding: chunked
<
{"error_code":40401,"message":"Topic eric not present in metadata after 60000 ms."}

• Connection #0 to host localhost left intact
• Closing connection 0

[CLAassistant]

All committers have signed the CLA.

[dimitarndimitrov]

It's not ideal that we have to jump through such hacky hoops to return an HTTP 4xx for this.

@ehumber Do you know why this doesn't result in UnknownTopicOrPartitionException? If not, can we ask the Kafka team? This sounds like something that should be handled on the Kafka end.

[ijuma]

Yeah, this doesn't seem great. There is an open PR that is related, but doesn't solve the underlying problem: apache/kafka#7283

Maybe we can piggyback on that.

[ehumber]

I don't think the Kafka code can no longer return a TimeoutException - this is a documented response and I'm sure lots of people have error handling code that is looking for a TimeoutException as an error here rather than a UnknownTopicOrPartitionException. So we need to be able to work out if it's the TimeoutException we're looking for.

https://github.com/apache/kafka/pull/7283/files#r850452641 would mean we can do a TimeoutException.getCause() to check, which would be nicer. The challenge with this is if the user is on a back level Kafka, we don't have any more information than we do now, so REST would still throw the 500.

An alternative (also not nice, but reduces the scope) is to put an extra exception mapper that does the same thing as above, but in https://github.com/confluentinc/kafka-rest/blob/1bb1595d6e8e21150b2bca1913a21634cd79985b/kafka-rest/src/main/java/io/confluent/kafkarest/response/StreamingResponse.java#L96, looking for the TimeoutException. That at least limits it to the produce call, rather than all TimeoutExceptions that rest-utils might handle.

Thoughts?

[ijuma]

The challenge with this is if the user is on a back level Kafka, we don't have any more information than we do now, so REST would still throw the 500.

@ehumber We control the version of kafka clients in rest-utils and the proposed fix is on the clients. So, I don't see how that's a problem.

[ehumber]

The challenge with this is if the user is on a back level Kafka, we don't have any more information than we do now, so REST would still throw the 500.

@ehumber We control the version of kafka clients in rest-utils and the proposed fix is on the clients. So, I don't see how that's a problem.

Good point :)

[ehumber]

https://confluentinc.atlassian.net/browse/KREST-5634 as a follow on.

I think we should make our user experience slightly nicer with this slightly not nice but safe change, and then revisit the Kafka side exception in the future.

@dimitarndimitrov are you happy to approve?

[dimitarndimitrov]

+1.

Thank you for the follow-up on this!