-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-18659: librdkafka compressed produce fails unless api versions returns produce v0 #18727
base: trunk
Are you sure you want to change the base?
Changes from 1 commit
a252374
243771e
8a5d335
6420650
b38a7ef
593db8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,10 @@ | |
"type": "request", | ||
"listeners": ["broker"], | ||
"name": "ProduceRequest", | ||
// Versions 0-2 were removed in Apache Kafka 4.0, Version 3 is the new baseline. | ||
// Versions 0-2 were removed in Apache Kafka 4.0, version 3 is the new baseline. Due to a bug in librdkafka, | ||
// these versions have to be included in the api versions response (see KAFKA-18659), which means we cannot exclude | ||
// them from `validVersions`. Instead, we reject requests with such versions in `KafkaApis` by returning | ||
// `UnsupportedVersion` errors. | ||
// | ||
// Version 1 and 2 are the same as version 0. | ||
// | ||
|
@@ -44,7 +47,7 @@ | |
// transaction V2 (KIP_890 part 2) is enabled, the produce request will also include the function for a | ||
// AddPartitionsToTxn call. If V2 is disabled, the client can't use produce request version higher than 11 within | ||
// a transaction. | ||
"validVersions": "3-12", | ||
"validVersions": "0-12", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of bringing back v0 and v1, could we just customize the minVersion of the produce ApiKey in ApiResponse? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am indeed exploring something involving customizing ApiKey/ApiResponse (that's what I meant when I said that there may be a cleaner way to make the change). But I was planning to leave 0-12 here and reduce the range within the code. The main advantage is that you get appropriate errors if you try to produce with v0-2. The option you suggest would result in a disconnection. I think a disconnection is too confusing if the given version is included as part of the api versions response. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that it's nearly impossible to make the experience for these super ancient clients good. I was more thinking about client developers implementing the kafka protocol. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we could make an effort to send a more meaningful error code for produce v0-v2. But since they are not really supported on the broker, just doing a disconnect like any other unsupported version also seems reasonable. |
||
"flexibleVersions": "9+", | ||
"fields": [ | ||
{ "name": "TransactionalId", "type": "string", "versions": "3+", "nullableVersions": "3+", "default": "null", "entityType": "transactionalId", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -374,6 +374,11 @@ class KafkaApis(val requestChannel: RequestChannel, | |
*/ | ||
def handleProduceRequest(request: RequestChannel.Request, requestLocal: RequestLocal): Unit = { | ||
val produceRequest = request.body[ProduceRequest] | ||
// See `ProduceRequest.MIN_VERSION` for details on why we need to do this | ||
if (produceRequest.version < ProduceRequest.MIN_VERSION) { | ||
requestHelper.sendErrorResponseMaybeThrottle(request, Errors.UNSUPPORTED_VERSION.exception()) | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
|
||
if (RequestUtils.hasTransactionalRecords(produceRequest)) { | ||
val isAuthorizedTransactional = produceRequest.transactionalId != null && | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bug fix is unrelated to the main change in this PR.