You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "FrankYang0529 (via GitHub)" <gi...@apache.org> on 2025/05/26 12:47:42 UTC
[PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
FrankYang0529 opened a new pull request, #19815:
URL: https://github.com/apache/kafka/pull/19815
The mapKey optimisation can be used in some KIP-932 RPC schemas to improve efficiency of some key-based accesses.
For ShareFetch, ShareAcknowledge, ShareGroupHeartbeat and ShareGroupDescribe, the v0 RPCs are already released and cannot be retrospectively optimized. This PR changes following RPCs:
* DeleteShareGroupOffsetsRequest
* DeleteShareGroupStateRequest
* DescribeShareGroupOffsetsRequest
* InitializeShareGroupStateRequest
* ReadShareGroupStateRequest
* ReadShareGroupStateSummaryRequest
* WriteShareGroupStateRequest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #19815:
URL: https://github.com/apache/kafka/pull/19815#discussion_r2113705265
##########
share-coordinator/src/test/java/org/apache/kafka/coordinator/share/ShareCoordinatorShardTest.java:
##########
@@ -451,15 +451,15 @@ public void testWriteStateFencedLeaderEpochError() {
.setFirstOffset(11)
.setLastOffset(20)
.setDeliveryCount((short) 1)
- .setDeliveryState((byte) 0)))))));
+ .setDeliveryState((byte) 0)))).iterator()))).iterator()));
Review Comment:
nit: indentation
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorShard.java:
##########
@@ -714,9 +714,11 @@ public CoordinatorResult<DeleteShareGroupOffsetsResultHolder, CoordinatorRecord>
);
}
+ DeleteShareGroupStateRequestData.DeleteStateDataCollection topicCollection = new DeleteShareGroupStateRequestData.DeleteStateDataCollection();
+ deleteShareGroupStateRequestTopicsData.forEach(d -> topicCollection.add(d.duplicate()));
Review Comment:
Is this duplication really necessary?
##########
share-coordinator/src/main/java/org/apache/kafka/coordinator/share/ShareCoordinatorService.java:
##########
@@ -807,9 +815,10 @@ public CompletableFuture<DeleteShareGroupStateResponseData> deleteState(RequestC
DeleteShareGroupStateRequestData requestForCurrentPartition = new DeleteShareGroupStateRequestData()
.setGroupId(groupId)
- .setTopics(List.of(new DeleteShareGroupStateRequestData.DeleteStateData()
+ .setTopics(new DeleteShareGroupStateRequestData.DeleteStateDataCollection(
+ List.of(new DeleteShareGroupStateRequestData.DeleteStateData()
.setTopicId(topicId)
- .setPartitions(List.of(partitionData))));
+ .setPartitions(new DeleteShareGroupStateRequestData.PartitionDataCollection(List.of(partitionData.duplicate()).iterator()))).iterator()));
Review Comment:
I'm trying to decide whether this is really worth making so many changes to `mapKey`. There's a lot of added complexity in situations like this. In test cases, it's not really that important. In the main code paths, it's more of a concern. wdyt?
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -3812,7 +3814,7 @@ class KafkaApis(val requestChannel: RequestChannel,
.setErrorMessage(Errors.TOPIC_AUTHORIZATION_FAILED.message())
)
} else {
- authorizedTopics.add(topic)
+ authorizedTopics.add(topic.duplicate())
Review Comment:
And here.
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -3696,14 +3696,16 @@ class KafkaApis(val requestChannel: RequestChannel,
requestContext,
DESCRIBE,
TOPIC,
- groupDescribeOffsetsRequest.topics.asScala
+ groupDescribeOffsetsRequest.topics.valuesList.asScala
)(_.topicName)
+ val topicCollection = new DescribeShareGroupOffsetsRequestData.DescribeShareGroupOffsetsRequestTopicCollection()
+ authorizedTopics.foreach(t => topicCollection.add(t.duplicate))
Review Comment:
I don't see why we now need duplication where previously we did not. Could you explain? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #19815:
URL: https://github.com/apache/kafka/pull/19815#issuecomment-2930842759
@dajac @AndrewJSchofield Thanks for the review. I think we only need to optimize the case like `AlterShareGroupOffsetsResponse.java` which tries to use `HashMap` to build the response. We can do enhancement by `mapKey`, so it can use the generated collection directly. I go through all request / response. We probably need to modify `AlterShareGroupOffsetsResponse.json`. If you all agree with it, I can update it in this PR.
https://github.com/apache/kafka/pull/18929#discussion_r2091058650
cc @chia7712
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield merged PR #19815:
URL: https://github.com/apache/kafka/pull/19815
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #19815:
URL: https://github.com/apache/kafka/pull/19815#issuecomment-2918807738
@FrankYang0529 Please can you merge latest changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #19815:
URL: https://github.com/apache/kafka/pull/19815#issuecomment-2931701446
@FrankYang0529 I have left a comment in the KAFKA-19290 that using `mapKey` does not break the protocol compatibility. Please also check its usage for ShareFetch, ShareAcknowledge, ShareGroupHeartbeat, and ShareGroupDescribe if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #19815:
URL: https://github.com/apache/kafka/pull/19815#issuecomment-2931028111
> @dajac @AndrewJSchofield Thanks for the review. I think we only need to optimize the case like `AlterShareGroupOffsetsResponse.java` which tries to use `HashMap` to build the response. We can do enhancement by `mapKey`, so it can use the generated collection directly. I go through all request / response. We probably need to modify `AlterShareGroupOffsetsResponse.json`. If you all agree with it, I can update it in this PR.
>
> [#18929 (comment)](https://github.com/apache/kafka/pull/18929#discussion_r2091058650)
>
> cc @chia7712
Sounds good to me. I agree that the most benefit is for accessing the data by key. I'm happy with you updating that response JSON schema.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #19815:
URL: https://github.com/apache/kafka/pull/19815#issuecomment-2949123414
@AndrewJSchofield I'm waiting for CI result. Essentially, it's ready. I already change all share group RPCs which can add `mapKey`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on PR #19815:
URL: https://github.com/apache/kafka/pull/19815#issuecomment-2949114322
@FrankYang0529 Is this still WIP? The PR title still contains "(wip)".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19290: Exploit mapKey optimisation in protocol requests and responses (wip) [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #19815:
URL: https://github.com/apache/kafka/pull/19815#issuecomment-2909952792
@FrankYang0529 Thanks for the patch. Could you highlight the places where using `mapKey` is useful? It seems that most of the changes are unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org