KAFKA-14402: Update AddPartitionsToTxn protocol to batch and handle verifyOnly requests by jolshan · Pull Request #13231 · apache/kafka

jolshan · 2023-02-10T19:18:20Z

Part 1 of KIP-890

I've updated the API spec and related classes.

Clients should only be able to send up to version 3 requests and that is enforced by using a client builder.

Requests > 4 only require cluster permissions as they are initiated from other brokers.

I've added tests for the batched requests and for the verifyOnly mode.

Also -- minor change to the KafkaApis method to properly match the request name.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

jeffkbkim

Thanks for the PR! left some comments

jolshan · 2023-02-23T00:40:16Z

I've added the changes to the API spec
-- verify only is now a transaction level config
-- top level error is added to the response

I've added builders to the request and tried to simplify some of the methods.
I've also addressed the verifyOnly case where some partitions are in the txn and others are not.

I will update the KIP to reflect some of these changes (especially with respect to the API spec)

I still need to address the unstable API change, but that will require a pull from master.

jolshan · 2023-02-23T01:06:05Z

Took a quick look at the unstable api change. Looks like some integration tests built specifically for v4 fail with org.apache.kafka.common.errors.InvalidRequestException: Received request api key ADD_PARTITIONS_TO_TXN with version 4 which is not enabled

I will need to look into this.

dajac · 2023-02-23T06:19:38Z

Took a quick look at the unstable api change. Looks like some integration tests built specifically for v4 fail with org.apache.kafka.common.errors.InvalidRequestException: Received request api key ADD_PARTITIONS_TO_TXN with version 4 which is not enabled

I will need to look into this.

@jolshan I suppose that you have to enable unstable apis in your new integration tests.

guozhangwang

Made a new pass according to the newly updated KIP, do not have further comments.

guozhangwang · 2023-02-24T23:38:01Z

@dajac @hachikuji if you do not have further comments, we can proceed and merge it then?

dajac

@jolshan Thanks for the PR. I made a first pass and left some comments.

dajac · 2023-02-27T07:44:20Z

        public void handleResponse(AbstractResponse response) {
            AddPartitionsToTxnResponse addPartitionsToTxnResponse = (AddPartitionsToTxnResponse) response;
-            Map<TopicPartition, Errors> errors = addPartitionsToTxnResponse.errors();
+            Map<TopicPartition, Errors> errors = addPartitionsToTxnResponse.errors().get(AddPartitionsToTxnResponse.V3_AND_BELOW_TXN_ID);


nit: I suppose that errors should never be null here. I wonder if we should still check it. What do you think?

What should we do if the check fails? Just have a better error message thrown?

Yeah, the check is probably not necessary. By the way, I find the idea of having V3_AND_BELOW_TXN_ID for old version a bit confusing. I was wondering if using addPartitionsToTxnResponse.data().resultsByTopicV3AndBelow() would be a better alternative here. We only iterate over the Map so the Map is not strictly required here. Have you considered something like this?

I was told not to have v3 and below specific methods from Jason because the v3 case should generalize to a single version of the v4 case and that should make it easy to use methods for both.

However, if we really think this is an issue. I guess we can change the approach again. I'm just not sure the experience of errors only applying to v4+. Any ideas there besides changing the method name to express it should only be used in v4+?

Understood. Let's keep it as it is then.

I agree that v3 case should generalized to a single item of the v4 case. It is just unfortunate that we don't have the transaction id in v3 response so we have to use an empty string for it. I suppose that it is the way it is.

Yeah. It really is unfortunate. 😞

… for top level errors, minor changes

dajac

@jolshan Thanks for the update. I left some nits. We should be good to go once they are addressed.

dajac

LGTM, thanks for the patch. I left two suggestions for follow-ups. We can do those in separate PRs. This one is large enough.

dajac · 2023-03-07T15:49:00Z

    ApiKeys.INIT_PRODUCER_ID -> ((resp: InitProducerIdResponse) => resp.error),
    ApiKeys.WRITE_TXN_MARKERS -> ((resp: WriteTxnMarkersResponse) => resp.errorsByProducerId.get(producerId).get(tp)),
-    ApiKeys.ADD_PARTITIONS_TO_TXN -> ((resp: AddPartitionsToTxnResponse) => resp.errors.get(tp)),
+    ApiKeys.ADD_PARTITIONS_TO_TXN -> ((resp: AddPartitionsToTxnResponse) => resp.errors.get(AddPartitionsToTxnResponse.V3_AND_BELOW_TXN_ID).get(tp)),


As a follow-up: We should cover the new version here as well.

The new version doesn't really use authorizer, so I wasn't sure if it was needed.

I guess it uses authHelper.authorizeClusterOperation(request, CLUSTER_ACTION)

dajac · 2023-03-07T15:50:16Z

  }

+  @Test
+  def testBatchedAddPartitionsToTxnRequest(): Unit = {


As a follow-up: It seems that the test coverage is pretty low for this API here. It would be great if we could extend it. e.g. authorization failures, validation failures, etc.

jolshan · 2023-03-07T17:28:10Z

for follow ups: https://issues.apache.org/jira/browse/KAFKA-14790

…mnative#942) Main changes: - Adapt to the new `AddPartitionsToTxnRequest` from apache/kafka#13231 (KIP-890) - Support the new `DescribeTopicPartitions` request from apache/kafka#14612 (KIP-966), which is required by some admin APIs Other changes: - apache/kafka#13760 will retry when `deleteRecords` returns a retriable error, change the error code to `INVALID_REQUEST`

jolshan added 6 commits February 3, 2023 11:57

basic batching

4f760d6

cleaner batching

70d11ba

bug fixes

a8d6c00

add verifyOnly and auth changes

47f0fff

more tests

4c4d99e

fixes

4ad3df0

jeffkbkim reviewed Feb 14, 2023

View reviewed changes

hachikuji reviewed Feb 15, 2023

View reviewed changes

guozhangwang reviewed Feb 16, 2023

View reviewed changes

dajac reviewed Feb 17, 2023

View reviewed changes

Comment thread clients/src/main/resources/common/message/AddPartitionsToTxnRequest.json Outdated

hachikuji reviewed Feb 17, 2023

View reviewed changes

Comment thread clients/src/main/resources/common/message/AddPartitionsToTxnRequest.json

jolshan added 3 commits February 21, 2023 13:46

update request json

3f0c89c

new builders

791d56b

Fix up requests/responses

f8dbaa1

jolshan force-pushed the kafka-14402 branch from f7e171d to 03b99de Compare February 23, 2023 00:43

Per Partition error codes for verify only

2c7d25c

jolshan force-pushed the kafka-14402 branch from 03b99de to 2c7d25c Compare February 23, 2023 00:49

Merge branch 'trunk' of github.com:apache/kafka into kafka-14402

4aa0417

jolshan added 3 commits February 23, 2023 14:15

unstable apis support

19dbfe6

Fix tests

42486de

just change the config for the test

32ca9bb

guozhangwang approved these changes Feb 24, 2023

View reviewed changes

dajac reviewed Feb 27, 2023

View reviewed changes

jolshan added 3 commits March 1, 2023 15:13

Nits, changed to one callback, added kafka apis test for batched mode

a7abdae

Redo callbacks by splitting into two methods, clean up error handling…

fd1f92b

… for top level errors, minor changes

test fix

4589d36

dajac reviewed Mar 3, 2023

View reviewed changes

Small changes

ac16b13

dajac approved these changes Mar 7, 2023

View reviewed changes

jolshan merged commit 29a1a16 into apache:trunk Mar 7, 2023

Conversation

jolshan commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

jeffkbkim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jolshan commented Feb 23, 2023

Uh oh!

jolshan commented Feb 23, 2023

Uh oh!

dajac commented Feb 23, 2023

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Feb 24, 2023

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jolshan commented Feb 10, 2023 •

edited

Loading