KAFKA-16713: Define initial set of RPCs for KIP-932 by AndrewJSchofield · Pull Request #16022 · apache/kafka

AndrewJSchofield · 2024-05-21T17:09:19Z

This PR defines the initial set of RPCs for KIP-932. The RPCs for the admin client and state management are not in this PR.

Committer Checklist (excluded from commit message)

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

dajac · 2024-05-21T17:25:57Z

Should we wait until we cut the 3.8 branch before merging those? If we don't wait, I suggest to mark all the new versions as unstable until they are implemented.

AndrewJSchofield · 2024-05-21T19:39:43Z

Yes, we will wait until we cut the 3.8 branch before merging.

apoorvmittal10

Thanks for the PR. LGTM, minor comments.

apoorvmittal10 · 2024-05-22T09:51:06Z

+ */
+package org.apache.kafka.common.errors;
+
+public class FencedStateEpochException extends ApiException {


Can we please have the comments for the exception class as like InvalidRecordStateException class defined in the PR. Sorry for missing this earlier but seems this error class is not defined in the KIP though others are.

apoorvmittal10 · 2024-05-22T10:01:38Z

+        );
+    }
+
+    public static List<ShareGroupDescribeResponseData.DescribedGroup> getErrorDescribedGroupList(


Generally I have received feedback of not including get in such getters so may be rename method to errorDescribedGroupList.

This class matches the similar ConsumerGroupDescribeRequest so I think it's best to leave it in the name of consistency.

apoorvmittal10 · 2024-05-22T10:04:14Z

  // Version 5 adds support for new error code TRANSACTION_ABORTABLE (KIP-890).
-  "validVersions": "0-5",
+  //
+  // Version 6 adds support for share groups (KIP-932).


We might want to update KIP with version 6 as KIP says version 5 is added for KIP-932.

Yes, I will do a KIP update once all of the IDs are confirmed by the merge. Essentially, KIP-890 overtook KIP-932.

omkreddy · 2024-05-22T13:58:57Z

looks compilation failure after KAFKA-7632: Support Compression Levels (KIP-390) (#15516). Can we rebase the PR?

[2024-05-22T13:44:38.423Z] > Task :clients:compileTestJava

[2024-05-22T13:44:38.423Z] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-16022/clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java:1417: error: no suitable method found for withRecords(CompressionType,SimpleRecord)

[2024-05-22T13:44:38.423Z]         MemoryRecords records = MemoryRecords.withRecords(CompressionType.NONE, new SimpleRecord("blah".getBytes()));

[2024-05-22T13:44:38.423Z]                                              ^

[2024-05-22T13:44:38.423Z]     method MemoryRecords.withRecords(Compression,SimpleRecord...) is not applicable

[2024-05-22T13:44:38.423Z]       (argument mismatch; CompressionType cannot be converted to Compression)

[2024-05-22T13:44:38.423Z]     method MemoryRecords.withRecords(Compression,int,SimpleRecord...) is not applicable

[2024-05-22T13:44:38.423Z]       (argument mismatch; CompressionType cannot be converted to Compression)

[2024-05-22T13:44:38.423Z]     method MemoryRecords.withRecords(byte,Compression,SimpleRecord...) is not applicable

[2024-05-22T13:44:38.423Z]       (argument mismatch; CompressionType cannot be converted to byte)

[2024-05-22T13:44:38.423Z]     method MemoryRecords.withRecords(long,Compression,SimpleRecord...) is not applicable

[2024-05-22T13:44:38.423Z]       (argument mismatch; CompressionType cannot be converted to long)

[2024-05-22T13:44:38.423Z]     method MemoryRecords.withRecords(byte,long,Compression,SimpleRecord...) is not applicable

[2024-05-22T13:44:38.423Z]       (argument mismatch; CompressionType cannot be converted to byte)

[2024-05-22T13:44:38.423Z]     method MemoryRecords.withRecords(long,Compression,Integer,SimpleRecord...) is not applicable

[2024-05-22T13:44:38.423Z]       (argument mismatch; CompressionType cannot be converted to long)

[2024-05-22T13:44:38.423Z]     method MemoryRecords.withRecords(byte,long,Compression,TimestampType,SimpleRecord...) is not applicable

[2024-05-22T13:44:38.423Z]       (argument mismatch; CompressionType cannot be converted to byte)

[2024-05-22T13:44:38.423Z]     method MemoryRecords.withRecords(byte,long,Compression,TimestampType,long,short,int,int,boolean,SimpleRecord...) is not applicable

[2024-05-22T13:44:38.423Z]       (argument mismatch; CompressionType cannot be converted to byte)

AndrewJSchofield · 2024-05-24T08:28:59Z

There are 2 test failures introduced by this PR. I'll get them fixed.

omkreddy

@AndrewJSchofield Thanks for the PR. LGTM

omkreddy · 2024-06-03T06:12:44Z

Test failures are not related to this PR: https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-16022/6/tests

This PR defines the initial set of RPCs for KIP-932. The RPCs for the admin client and state management are not in this PR. Reviewers: Apoorv Mittal <amittal@confluent.io>, Manikumar Reddy <manikumar.reddy@gmail.com>

…6022)" This reverts commit 8f82f14.

…16022)" This reverts commit d6bff1f.

This PR defines the initial set of RPCs for KIP-932. The RPCs for the admin client and state management are not in this PR. Reviewers: Apoorv Mittal <amittal@confluent.io>, Manikumar Reddy <manikumar.reddy@gmail.com>

chia7712 · 2024-06-14T07:12:01Z

+        return counts;
+    }
+
+    public LinkedHashMap<TopicIdPartition, ShareFetchResponseData.PartitionData> responseData(Map<Uuid, String> topicNames) {


just curious. The cached responseData is created according to input topicNames. It will return same object even though we use different input, and hence I'm not sure how we will use it? If that ShareFetchResponse will share everywhere, it will be error-prone. If ShareFetchResponse is used as a local variable, the cache seems to be useless.

Please correct me if I misunderstand anything.

I agree, method will reply with previous parsed response irrespective of input. However I see similar issue in FetchResponse.java. Though for ShareFetch this method will be used only in tests for now but either we should fix or atleast write the expectations from the method in javadoc.

@adixitconfluent @AndrewJSchofield wdyt? I can write the javadoc with expectations in next PR if that we agrees with.

This is true, but it is derived from FetchResponse. @apoorvmittal10's suggestion works for me.

I see similar issue in FetchResponse.java.

yes, I have filed a jira https://issues.apache.org/jira/browse/KAFKA-16684 for it. Also, there is already a PR #15966

@apoorvmittal10's suggestion works for me.

sounds good to me too

junrao

@AndrewJSchofield : Thanks for the PR and sorry for the late review. Left a comment below.

junrao · 2024-07-26T22:50:52Z

+            "about": "First offset of batch of records to acknowledge."},
+          { "name": "LastOffset", "type": "int64", "versions": "0+",
+            "about": "Last offset (inclusive) of batch of records to acknowledge."},
+          { "name": "AcknowledgeTypes", "type": "[]int8", "versions": "0+",


@AndrewJSchofield : I am wondering why AcknowledgeTypes is an array instead of just an int8. This forces every record to be acked individually and I thought we want to optimize the case that multiple records are of the same ack type.

@junrao As it says in the KIP, AcknowledgeTypes can consist of a single value which applies to all entries in the batch, or a separate value for each offset in the batch. By making it an array, we have the option of optimising with a single entry in the common case where multiple records are of the same ack type, but we can also accommodate the case where they differ simply by adding more entries to the array.

omkreddy added the queues-for-kafka label May 22, 2024

omkreddy self-assigned this May 22, 2024

apoorvmittal10 approved these changes May 22, 2024

View reviewed changes

AndrewJSchofield force-pushed the KAFKA-16713 branch from dcf69eb to e9b9591 Compare May 23, 2024 15:43

AndrewJSchofield mentioned this pull request May 30, 2024

KAFKA-16715: Create KafkaShareConsumer interfaces #16134

Merged

3 tasks

AndrewJSchofield added 4 commits May 31, 2024 10:44

KAFKA-16713: Define initial set of RPCs for KIP-932

063b328

Review comments

49d12a9

Rebased

ba56986

Fix test failures

9cbfc2f

AndrewJSchofield force-pushed the KAFKA-16713 branch from f917fd4 to 9cbfc2f Compare May 31, 2024 09:45

omkreddy approved these changes Jun 3, 2024

View reviewed changes

omkreddy merged commit 8f82f14 into apache:trunk Jun 3, 2024

AndrewJSchofield deleted the KAFKA-16713 branch June 3, 2024 07:41

brenden20 added a commit to brenden20/kafka that referenced this pull request Jun 3, 2024

Revert "KAFKA-16713: Define initial set of RPCs for KIP-932 (apache#1…

d6bff1f

…6022)" This reverts commit 8f82f14.

brenden20 added a commit to brenden20/kafka that referenced this pull request Jun 4, 2024

Reapply "KAFKA-16713: Define initial set of RPCs for KIP-932 (apache#…

c96499a

…16022)" This reverts commit d6bff1f.

chia7712 reviewed Jun 14, 2024

View reviewed changes

junrao reviewed Jul 26, 2024

View reviewed changes

mumrah added KIP-932 Queues for Kafka and removed queues-for-kafka labels Sep 16, 2024

Conversation

AndrewJSchofield commented May 21, 2024

Committer Checklist (excluded from commit message)

Uh oh!

dajac commented May 21, 2024

Uh oh!

AndrewJSchofield commented May 21, 2024

Uh oh!

apoorvmittal10 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!

omkreddy commented May 22, 2024

Uh oh!

AndrewJSchofield commented May 24, 2024

Uh oh!

omkreddy left a comment

Choose a reason for hiding this comment

Uh oh!

omkreddy commented Jun 3, 2024

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!

junrao 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants