Skip to content
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
36cb999
KAFKA-17747: Add compute topic and group hash
FrankYang0529 Apr 21, 2025
f4c9749
KAFKA-17747: add guava license
FrankYang0529 Apr 21, 2025
ced64d7
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 Apr 21, 2025
7e00a14
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 Apr 28, 2025
77b99da
Revert "KAFKA-17747: add guava license"
FrankYang0529 Apr 28, 2025
aff55ce
KAFKA-17747: Move Murmur3 to org.apache.kafka.common.internals
FrankYang0529 Apr 28, 2025
7f7649a
KAFKA-17747: Replace guava with Murmur3
FrankYang0529 Apr 28, 2025
8fc5b33
remove unused package in import-control-group-coordinator.xml
FrankYang0529 Apr 28, 2025
d2427f9
remove sbt.json
FrankYang0529 Apr 28, 2025
b5eebbf
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 Apr 29, 2025
b6ccef3
Revert "KAFKA-17747: Move Murmur3 to org.apache.kafka.common.internals"
FrankYang0529 Apr 29, 2025
a1be1f6
KAFKA-17747: Replace Murmur3 with XXHash64
FrankYang0529 Apr 29, 2025
9b4294f
KAFKA-17747: Move static function to Utils
FrankYang0529 Apr 29, 2025
478dd98
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 Apr 30, 2025
9fc8e8e
KAFKA-17747: Replace ByteArrayOutputStream with ByteBufferOutputStream
FrankYang0529 Apr 30, 2025
167cd4a
KAFKA-17747: Add comment
FrankYang0529 Apr 30, 2025
6e219a9
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 May 1, 2025
08cdd37
KAFKA-17747: Change index to length in rack string
FrankYang0529 May 1, 2025
527b387
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 May 4, 2025
cde92d8
KAFKA-17747: remove : and , in rack string and change streams to for-…
FrankYang0529 May 4, 2025
b238375
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 May 6, 2025
a1b658a
KAFKA-17747: change to use hash4j
FrankYang0529 May 13, 2025
ed7b450
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 May 13, 2025
44c1337
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 May 13, 2025
f1ea74b
address comment
FrankYang0529 May 13, 2025
bb1edb3
address comment
FrankYang0529 May 14, 2025
3382702
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 May 14, 2025
c031a29
KAFKA-17747: handle empty map in computeGroupHash
FrankYang0529 May 14, 2025
ec7024c
Merge branch 'trunk' into KAFKA-17747-2
FrankYang0529 May 14, 2025
cf35b7a
address comment
FrankYang0529 May 14, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions LICENSE-binary
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ License Version 2.0:
- commons-lang3-3.12.0
- commons-logging-1.3.2
- commons-validator-1.9.0
- hash4j-0.22.0
- jackson-annotations-2.16.2
- jackson-core-2.16.2
- jackson-databind-2.16.2
Expand Down
2 changes: 2 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -1414,10 +1414,12 @@ project(':group-coordinator') {
implementation project(':coordinator-common')
implementation libs.jacksonDatabind
implementation libs.jacksonJDK8Datatypes
implementation libs.lz4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it. Thanks.

implementation libs.metrics
implementation libs.hdrHistogram
implementation libs.re2j
implementation libs.slf4jApi
implementation libs.hash4j
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that we need to update the license file too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I added it to LICENSE-binary and python committer-tools/verify_license.py can pass. Do I need to modify other files? Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. Thanks.


testImplementation project(':clients').sourceSets.test.output
testImplementation project(':server-common').sourceSets.test.output
Expand Down
2 changes: 2 additions & 0 deletions checkstyle/import-control-group-coordinator.xml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@

<subpackage name="coordinator">
<subpackage name="group">
<allow pkg="net.jpountz.xxhash" />
<allow pkg="org.apache.kafka.clients.consumer" />
<allow pkg="org.apache.kafka.common.annotation" />
<allow pkg="org.apache.kafka.common.config" />
Expand All @@ -76,6 +77,7 @@
<allow pkg="org.apache.kafka.coordinator.common" />
<allow pkg="org.apache.kafka.coordinator.common.runtime" />
<allow pkg="com.google.re2j" />
<allow pkg="com.dynatrace.hash4j.hashing" />
<allow pkg="org.apache.kafka.metadata" />
<subpackage name="metrics">
<allow pkg="com.yammer.metrics"/>
Expand Down
6 changes: 4 additions & 2 deletions gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ versions += [
// Also make sure the compression levels in org.apache.kafka.common.record.CompressionType are still valid
zstd: "1.5.6-10",
junitPlatform: "1.10.2",
hdrHistogram: "2.2.2"
hdrHistogram: "2.2.2",
hash4j: "0.22.0"
]

libs += [
Expand Down Expand Up @@ -225,5 +226,6 @@ libs += [
mavenArtifact: "org.apache.maven:maven-artifact:$versions.mavenArtifact",
zstd: "com.github.luben:zstd-jni:$versions.zstd",
httpclient: "org.apache.httpcomponents:httpclient:$versions.httpclient",
hdrHistogram: "org.hdrhistogram:HdrHistogram:$versions.hdrHistogram"
hdrHistogram: "org.hdrhistogram:HdrHistogram:$versions.hdrHistogram",
hash4j: "com.dynatrace.hash4j:hash4j:$versions.hash4j",
]
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,18 @@
import org.apache.kafka.common.protocol.ApiMessage;
import org.apache.kafka.coordinator.group.generated.ConsumerGroupCurrentMemberAssignmentValue;
import org.apache.kafka.coordinator.group.generated.ShareGroupCurrentMemberAssignmentValue;
import org.apache.kafka.image.ClusterImage;
import org.apache.kafka.image.TopicImage;
import org.apache.kafka.image.TopicsImage;
import org.apache.kafka.metadata.BrokerRegistration;
import org.apache.kafka.server.common.ApiMessageAndVersion;

import com.dynatrace.hash4j.hashing.HashStream64;
import com.dynatrace.hash4j.hashing.Hashing;
import com.google.re2j.Pattern;
import com.google.re2j.PatternSyntaxException;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -324,4 +329,84 @@ static void throwIfRegularExpressionIsInvalid(
regex, ex.getDescription()));
}
}

/**
* The magic byte used to identify the version of topic hash function.
*/
static final byte TOPIC_HASH_MAGIC_BYTE = 0x00;

/**
* Computes the hash of the topics in a group.
* <p>
* The computed hash value is stored as part of the metadata hash in the *GroupMetadataValue.
* <p>
* The hashing process involves the following steps:
* 1. Sort the topic hashes by topic name.
* 2. Write each topic hash in order.
*
* @param topicHashes The map of topic hashes. Key is topic name and value is the topic hash.
* @return The hash of the group.
*/
static long computeGroupHash(Map<String, Long> topicHashes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: public?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this function will only be used in GroupMetadataManager like throwIfRegularExpressionIsInvalid. I think we can add public when we need it. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. It works for me.

// Sort entries by topic name
List<Map.Entry<String, Long>> sortedEntries = new ArrayList<>(topicHashes.entrySet());
sortedEntries.sort(Map.Entry.comparingByKey());

HashStream64 hasher = Hashing.xxh3_64().hashStream();
for (Map.Entry<String, Long> entry : sortedEntries) {
hasher.putLong(entry.getValue());
}

return hasher.getAsLong();
}

/**
* Computes the hash of the topic id, name, number of partitions, and partition racks by XXHash64.
* <p>
* The computed hash value for the topic is utilized in conjunction with the {@link #computeGroupHash(Map)}
* method and is stored as part of the metadata hash in the *GroupMetadataValue.
* It is important to note that if the hash algorithm is changed, the magic byte must be updated to reflect the
* new hash version.
* <p>
* The hashing process involves the following steps:
* 1. Write a magic byte to denote the version of the hash function.
* 2. Write the hash code of the topic ID.
* 3. Write the topic name.
* 4. Write the number of partitions associated with the topic.
* 5. For each partition, write the partition ID and a sorted list of rack identifiers.
* - Rack identifiers are formatted as "<length1><value1><length2><value2>" to prevent issues with simple separators.
*
* @param topicImage The topic image.
* @param clusterImage The cluster image.
* @return The hash of the topic.
*/
static long computeTopicHash(TopicImage topicImage, ClusterImage clusterImage) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add documentation to remind developers that the hash is stored as part of the state. Changing the implementation of the hashing function may break compatibility with existing states.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the hashing function is ever changed, is there a version field that should be updated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the hashing function is ever changed, is there a version field that should be updated?

yes, there is a magic byte as version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: public?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

HashStream64 hasher = Hashing.xxh3_64().hashStream();
hasher = hasher.putByte(TOPIC_HASH_MAGIC_BYTE) // magic byte
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could you please put .putByte(TOPIC_HASH_MAGIC_BYTE) // magic byte on a new line. It is easier to read if it is aligned with the other ones.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it.

.putLong(topicImage.id().hashCode()) // topic ID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder about this one. It may be better to hash it as two longs (getMostSignificantBits and getLeastSignificantBits). What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. This can use whole data of Uuid to calculate topic hash. It's better.

.putString(topicImage.name()) // topic name
.putInt(topicImage.partitions().size()); // number of partitions

for (int i = 0; i < topicImage.partitions().size(); i++) {
hasher = hasher.putInt(i); // partition id
// The rack string combination cannot use simple separator like ",", because there is no limitation for rack character.
// If using simple separator like "," it may hit edge case like ",," and ",,," / ",,," and ",,".
// Add length before the rack string to avoid the edge case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rather put this after // Format: "<length><value>"? It seems more related to that part of the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it.

List<String> racks = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I wonder whether we could reuse this list. We could declare it before the loop and clear it before using it. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I initialize racks outside the for-loop and call clear before adding data.

for (int replicaId : topicImage.partitions().get(i).replicas) {
BrokerRegistration broker = clusterImage.broker(replicaId);
if (broker != null) {
Optional<String> rackOptional = broker.rack();
rackOptional.ifPresent(racks::add);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We could combine those two lines into one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it.

}
}

Collections.sort(racks);
for (String rack : racks) {
// Format: "<length><value>"
hasher = hasher.putInt(rack.length()).putString(rack);
}
}
return hasher.getAsLong();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should we put an empty line before the return?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.kafka.coordinator.group;

import org.apache.kafka.common.Uuid;
import org.apache.kafka.image.MetadataImage;

import com.dynatrace.hash4j.hashing.Hashing;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.io.IOException;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.stream.Stream;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotEquals;

public class UtilsTest {
private static final Uuid FOO_TOPIC_ID = Uuid.randomUuid();
private static final String FOO_TOPIC_NAME = "foo";
private static final String BAR_TOPIC_NAME = "bar";
private static final int FOO_NUM_PARTITIONS = 2;
private static final MetadataImage FOO_METADATA_IMAGE = new MetadataImageBuilder()
.addTopic(FOO_TOPIC_ID, FOO_TOPIC_NAME, FOO_NUM_PARTITIONS)
.addRacks()
.build();

@Test
void testComputeTopicHash() throws IOException {
long result = Utils.computeTopicHash(FOO_METADATA_IMAGE.topics().getTopic(FOO_TOPIC_ID), FOO_METADATA_IMAGE.cluster());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a usage perspective, I wonder whether we should just pass the topic name and the metadata image to the method to compute the hash. I suppose that we will use it like this in the end but I am not 100% sure. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If input is topic name and metadata image, we need to handle null TopicImage in computeTopicHash. Do we want to handle this error in it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could return 0 when the topic does not exist. Would it work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it works. I also check whether topic map is empty computeGroupHash. If it's, return 0.


long expected = Hashing.xxh3_64().hashStream()
.putByte((byte) 0) // magic byte
.putLong(FOO_TOPIC_ID.hashCode()) // topic ID
.putString(FOO_TOPIC_NAME) // topic name
.putInt(FOO_NUM_PARTITIONS) // number of partitions
.putInt(0) // partition 0
.putInt(5) // length of rack0
.putString("rack0") // The first rack in partition 0
.putInt(5) // length of rack1
.putString("rack1") // The second rack in partition 0
.putInt(1) // partition 1
.putInt(5) // length of rack0
.putString("rack1") // The first rack in partition 1
.putInt(5) // length of rack1
.putString("rack2") // The second rack in partition 1
.getAsLong();
assertEquals(expected, result);
}

@Test
void testComputeTopicHashWithDifferentMagicByte() throws IOException {
long result = Utils.computeTopicHash(FOO_METADATA_IMAGE.topics().getTopic(FOO_TOPIC_ID), FOO_METADATA_IMAGE.cluster());

long expected = Hashing.xxh3_64().hashStream()
.putByte((byte) 1) // magic byte
.putLong(FOO_TOPIC_ID.hashCode()) // topic ID
.putString(FOO_TOPIC_NAME) // topic name
.putInt(FOO_NUM_PARTITIONS) // number of partitions
.putInt(0) // partition 0
.putInt(5) // length of rack0
.putString("rack0") // The first rack in partition 0
.putInt(5) // length of rack1
.putString("rack1") // The second rack in partition 0
.putInt(1) // partition 1
.putInt(5) // length of rack0
.putString("rack1") // The first rack in partition 1
.putInt(5) // length of rack1
.putString("rack2") // The second rack in partition 1
.getAsLong();
assertNotEquals(expected, result);
}

@Test
void testComputeTopicHashWithDifferentPartitionOrder() throws IOException {
long result = Utils.computeTopicHash(FOO_METADATA_IMAGE.topics().getTopic(FOO_TOPIC_ID), FOO_METADATA_IMAGE.cluster());

long expected = Hashing.xxh3_64().hashStream()
.putByte((byte) 1) // magic byte
.putLong(FOO_TOPIC_ID.hashCode()) // topic ID
.putString(FOO_TOPIC_NAME) // topic name
.putInt(FOO_NUM_PARTITIONS) // number of partitions
.putInt(1) // partition 1
.putInt(5) // length of rack0
.putString("rack1") // The first rack in partition 1
.putInt(5) // length of rack1
.putString("rack2") // The second rack in partition 1
.putInt(0) // partition 0
.putInt(5) // length of rack0
.putString("rack0") // The first rack in partition 0
.putInt(5) // length of rack1
.putString("rack1") // The second rack in partition 0
.getAsLong();
assertNotEquals(expected, result);
}

@Test
void testComputeTopicHashWithDifferentRackOrder() throws IOException {
long result = Utils.computeTopicHash(FOO_METADATA_IMAGE.topics().getTopic(FOO_TOPIC_ID), FOO_METADATA_IMAGE.cluster());

long expected = Hashing.xxh3_64().hashStream()
.putByte((byte) 0) // magic byte
.putLong(FOO_TOPIC_ID.hashCode()) // topic ID
.putString(FOO_TOPIC_NAME) // topic name
.putInt(FOO_NUM_PARTITIONS) // number of partitions
.putInt(0) // partition 0
.putInt(5) // length of rack0
.putString("rack1") // The second rack in partition 0
.putInt(5) // length of rack1
.putString("rack0") // The first rack in partition 0
.putInt(1) // partition 1
.putInt(5) // length of rack0
.putString("rack1") // The first rack in partition 1
.putInt(5) // length of rack1
.putString("rack2") // The second rack in partition 1
.getAsLong();
assertNotEquals(expected, result);
}

@ParameterizedTest
@MethodSource("differentFieldGenerator")
void testComputeTopicHashWithDifferentField(MetadataImage differentImage, Uuid topicId) throws IOException {
long result = Utils.computeTopicHash(FOO_METADATA_IMAGE.topics().getTopic(FOO_TOPIC_ID), FOO_METADATA_IMAGE.cluster());

assertNotEquals(
Utils.computeTopicHash(
differentImage.topics().getTopic(topicId),
differentImage.cluster()
),
result
);
}

private static Stream<Arguments> differentFieldGenerator() {
Uuid differentTopicId = Uuid.randomUuid();
return Stream.of(
Arguments.of(new MetadataImageBuilder() // different topic id
.addTopic(differentTopicId, FOO_TOPIC_NAME, FOO_NUM_PARTITIONS)
.addRacks()
.build(),
differentTopicId
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could you please put new MetadataImageBuilder() // different topic id on a new line too?

            Arguments.of(
                new MetadataImageBuilder() // different topic id
                    .addTopic(differentTopicId, FOO_TOPIC_NAME, FOO_NUM_PARTITIONS)
                    .addRacks()
                    .build(),
                differentTopicId
            ),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it. Thanks.

Arguments.of(new MetadataImageBuilder() // different topic name
.addTopic(FOO_TOPIC_ID, "bar", FOO_NUM_PARTITIONS)
.addRacks()
.build(),
FOO_TOPIC_ID
),
Arguments.of(new MetadataImageBuilder() // different partitions
.addTopic(FOO_TOPIC_ID, FOO_TOPIC_NAME, 1)
.addRacks()
.build(),
FOO_TOPIC_ID
),
Arguments.of(new MetadataImageBuilder() // different racks
.addTopic(FOO_TOPIC_ID, FOO_TOPIC_NAME, FOO_NUM_PARTITIONS)
.build(),
FOO_TOPIC_ID
)
);
}

@Test
void testComputeGroupHashWithDifferentOrder() {
Map<String, Long> ascendTopicHashes = new LinkedHashMap<>();
ascendTopicHashes.put(BAR_TOPIC_NAME, 123L);
ascendTopicHashes.put(FOO_TOPIC_NAME, 456L);

Map<String, Long> descendTopicHashes = new LinkedHashMap<>();
descendTopicHashes.put(FOO_TOPIC_NAME, 456L);
descendTopicHashes.put(BAR_TOPIC_NAME, 123L);
assertEquals(Utils.computeGroupHash(ascendTopicHashes), Utils.computeGroupHash(descendTopicHashes));
}

@Test
void testComputeGroupHashWithSameKeyButDifferentValue() {
Map<String, Long> map1 = Map.of(
BAR_TOPIC_NAME, 123L,
FOO_TOPIC_NAME, 456L
);

Map<String, Long> map2 = Map.of(
BAR_TOPIC_NAME, 456L,
FOO_TOPIC_NAME, 123L
);
assertNotEquals(Utils.computeGroupHash(map1), Utils.computeGroupHash(map2));
}
}