KAFKA-19846: Remove uuid to string conversion and vice-versa for memberId on broker #20819
Conversation
github-actions
bot
added
triage
| val shareFetchData = shareFetchRequest.shareFetchData(topicIdNames) | ||
| val forgottenTopics = shareFetchRequest.forgottenTopics(topicIdNames) | ||
|
|
||
| val newReqMetadata: ShareRequestMetadata = new ShareRequestMetadata(Uuid.fromString(memberId), shareSessionEpoch) |
There was a problem hiding this comment.
The tricky thing here is that previously we expected the member ID to be a UUID formatted in the way that the Kafka Java class does it. But, the RPCs define it as a string and there's no need for it to be formatted in that way, provided it's unique across the clients. We should validate the incoming member ID string to ensure that it's non-zero in length, and that it's length does not extend beyond the length of a human-readable UUID (36 characters).
Also, we use AAAAAAAAAAAAAAAAAAAAAAAA as the comparison value for no member ID, so that's a bad choice too. We could change the comparison value for no member ID to the empty string "", which the validation above would reject for use as a member ID, and then it would be permissible (but ill-advised) to use AAAAAAAAAAAAAAAAAAAAAAAA as the member ID, just because that's a well-known value.
There was a problem hiding this comment.
agreed and done
There was a problem hiding this comment.
Can you please help me understand regrading why we do not want the ShareRequestMetadata in the calls i.e. why do we need that chnage? I was expecting the non-uuid conversion of string, just only some validations as @AndrewJSchofield suggested.
| /** | ||
| * Returns true if this is a full share fetch request. | ||
| */ | ||
| public boolean isFull() { | ||
| return (this.epoch == INITIAL_EPOCH) || (this.epoch == FINAL_EPOCH); | ||
| } | ||
|
|
There was a problem hiding this comment.
This function was only utilized within the broker, hence I've removed this functionality from this class and added it to SharePartitionManager.java
| /** | ||
| * The member ID. | ||
| */ | ||
| private final Uuid memberId; |
There was a problem hiding this comment.
I was expecting a change here rather than removing the usage of ShareRequestMetadata
There was a problem hiding this comment.
ShareRequestMetadata is utilized in the client at multiple places such as ShareFetchRequest.java, ShareSessionHandler.java and ShareAcknowledgeRequest.java because memberId is treated as Uuid there, hence I did not touch this class. Besides, all the functionalities of ShareRequestMetadata that are utilized in the broker are static in nature, hence I got rid of it in the broker.
4 participants
About
This PR aims to resolve the bug when we have '/' or '+' in
memberIdwithin share fetch and share acknowledge RPCs. The bug arises primarily
because we do a
string -> Uuidconversion and vice-versa in the brokerfor
memberId. When a string containing '/' or '+' is converted toUuid, the
Base64.getUrlDecoder()is unable to decode the string to aUuid and gives an exception which is the reason for the bug.
On analysis, it looks like there is no efficiency added due to this
inter-conversion. Hence, we have gotten rid of the conversion from
string -> UuidinKafkaApisformemberId. Thus, everywhere on thebroker memberId always treated as string not a Uuid
While fixing this, I noticed that we don't need mapping in
ShareSessionContextclass, hence removed it.Testing
Added a few unit tests to validate the new functionality