Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,13 @@
import org.apache.kafka.server.common.GroupVersion;
import org.apache.kafka.server.common.KRaftVersion;
import org.apache.kafka.server.common.MetadataVersion;
import org.apache.kafka.server.common.ShareVersion;
import org.apache.kafka.server.common.StreamsVersion;
import org.apache.kafka.server.common.TestFeatureVersion;
import org.apache.kafka.server.common.TransactionVersion;
import org.apache.kafka.test.TestUtils;

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.junit.jupiter.params.ParameterizedTest;
Expand Down Expand Up @@ -271,6 +274,8 @@ public void testFormatWithOlderReleaseVersion() throws Exception {
}
}

// This test is disabled because all features are stable in Apache Kafka 4.2.
@Disabled
@Test
public void testFormatWithUnstableReleaseVersionFailsWithoutEnableUnstable() throws Exception {
try (TestEnv testEnv = new TestEnv(1)) {
Expand Down Expand Up @@ -391,6 +396,12 @@ public void testFeatureFlag(short version) throws Exception {
expected.add(new ApiMessageAndVersion(new FeatureLevelRecord().
setName(GroupVersion.FEATURE_NAME).
setFeatureLevel(GroupVersion.GV_1.featureLevel()), (short) 0));
expected.add(new ApiMessageAndVersion(new FeatureLevelRecord().
setName(ShareVersion.FEATURE_NAME).
setFeatureLevel(ShareVersion.SV_1.featureLevel()), (short) 0));
expected.add(new ApiMessageAndVersion(new FeatureLevelRecord().
setName(StreamsVersion.FEATURE_NAME).
setFeatureLevel(StreamsVersion.SV_1.featureLevel()), (short) 0));
if (version > 0) {
expected.add(new ApiMessageAndVersion(new FeatureLevelRecord().
setName(TestFeatureVersion.FEATURE_NAME).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,19 +114,19 @@ public enum MetadataVersion {
// Send FETCH version 18 in the replica fetcher (KIP-1166)
IBP_4_1_IV1(27, "4.1", "IV1", false),

// Enables share groups by default for new clusters (KIP-932).
IBP_4_2_IV0(28, "4.2", "IV0", false),

// Enables "streams" groups by default for new clusters (KIP-1071).
IBP_4_2_IV1(29, "4.2", "IV1", false);

//
// NOTE: MetadataVersions after this point are unstable and may be changed.
// If users attempt to use an unstable MetadataVersion, they will get an error unless
// they have set the configuration unstable.feature.versions.enable=true.
// Please move this comment when updating the LATEST_PRODUCTION constant.
//

// Enables share groups by default for new clusters (KIP-932).
IBP_4_2_IV0(28, "4.2", "IV0", false),

// Enables "streams" groups by default for new clusters (KIP-1071).
IBP_4_2_IV1(29, "4.2", "IV1", false);

// NOTES when adding a new version:
// Update the default version in @ClusterTest annotation to point to the latest version
// Change expected message in org.apache.kafka.tools.FeatureCommandTest in multiple places (search for "Change expected message")
Expand All @@ -145,7 +145,7 @@ public enum MetadataVersion {
* <strong>Think carefully before you update this value. ONCE A METADATA VERSION IS PRODUCTION,
* IT CANNOT BE CHANGED.</strong>
*/
public static final MetadataVersion LATEST_PRODUCTION = IBP_4_1_IV1;
public static final MetadataVersion LATEST_PRODUCTION = IBP_4_2_IV1;
// If you change the value above please also update
// LATEST_STABLE_METADATA_VERSION version in tests/kafkatest/version.py

Expand Down Expand Up @@ -372,7 +372,8 @@ public static MetadataVersion fromFeatureLevel(short version) {

// Testing only
public static MetadataVersion latestTesting() {
return VERSIONS[VERSIONS.length - 1];
// There is no separate testing version because all features are currently stable.
return LATEST_PRODUCTION;
Copy link
Member

Choose a reason for hiding this comment

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

Perhasp we could add IBP_4_3_IV0 to align the code with other 4.x branches? This would also make the patch easier to cherry-pick to trunk

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I did consider this, but there's no need for IBP_4_3_IV0 because there are no unstable features.

Copy link
Member Author

Choose a reason for hiding this comment

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

This sounds like it ought to be part of the release process :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made a PR against trunk #20977. Let me know if you prefer that cherry-picked into 4.2.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer #20977 because it helps to avoid the awkward disabled tag 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix the comments on that one then and will cherry-pick when it's ready.

}

public static MetadataVersion latestProduction() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import org.apache.kafka.common.protocol.ApiKeys;

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
Expand Down Expand Up @@ -90,16 +91,15 @@ public void testFromVersionString() {
assertEquals(IBP_4_1_IV0, MetadataVersion.fromVersionString("4.1-IV0", true));
assertEquals(IBP_4_1_IV1, MetadataVersion.fromVersionString("4.1-IV1", true));

// 4.2-IV1 is the latest production version in the 4.2 line
assertEquals(IBP_4_2_IV1, MetadataVersion.fromVersionString("4.2", true));
assertEquals(IBP_4_2_IV0, MetadataVersion.fromVersionString("4.2-IV0", true));
assertEquals(IBP_4_2_IV1, MetadataVersion.fromVersionString("4.2-IV1", true));

// Throws exception when unstableFeatureVersionsEnabled is false
assertEquals("Unknown metadata.version '4.2-IV0'. Supported metadata.version are: 3.3-IV3, 3.4-IV0, 3.5-IV0, 3.5-IV1, 3.5-IV2, "
+ "3.6-IV0, 3.6-IV1, 3.6-IV2, 3.7-IV0, 3.7-IV1, 3.7-IV2, 3.7-IV3, 3.7-IV4, 3.8-IV0, 3.9-IV0, 4.0-IV0, 4.0-IV1, 4.0-IV2, 4.0-IV3, 4.1-IV0, 4.1-IV1",
assertThrows(IllegalArgumentException.class, () -> fromVersionString("4.2-IV0", false)).getMessage());
assertEquals("Unknown metadata.version '4.2-IV1'. Supported metadata.version are: 3.3-IV3, 3.4-IV0, 3.5-IV0, 3.5-IV1, 3.5-IV2, "
+ "3.6-IV0, 3.6-IV1, 3.6-IV2, 3.7-IV0, 3.7-IV1, 3.7-IV2, 3.7-IV3, 3.7-IV4, 3.8-IV0, 3.9-IV0, 4.0-IV0, 4.0-IV1, 4.0-IV2, 4.0-IV3, 4.1-IV0, 4.1-IV1",
assertThrows(IllegalArgumentException.class, () -> fromVersionString("4.2-IV1", false)).getMessage());
assertEquals("Unknown metadata.version '4.3-IV0'. Supported metadata.version are: 3.3-IV3, 3.4-IV0, 3.5-IV0, 3.5-IV1, 3.5-IV2, "
+ "3.6-IV0, 3.6-IV1, 3.6-IV2, 3.7-IV0, 3.7-IV1, 3.7-IV2, 3.7-IV3, 3.7-IV4, 3.8-IV0, 3.9-IV0, 4.0-IV0, 4.0-IV1, 4.0-IV2, 4.0-IV3, 4.1-IV0, 4.1-IV1, 4.2-IV0, 4.2-IV1",
assertThrows(IllegalArgumentException.class, () -> fromVersionString("4.3-IV0", false)).getMessage());
}

@Test
Expand Down Expand Up @@ -249,6 +249,8 @@ public void testRegisterBrokerRecordVersion(MetadataVersion metadataVersion) {
assertEquals(expectedVersion, metadataVersion.registerBrokerRecordVersion());
}

// This test is disabled because all features are stable in Apache Kafka 4.2.
@Disabled
@Test
public void assertLatestProductionIsLessThanLatest() {
assertTrue(LATEST_PRODUCTION.ordinal() < MetadataVersion.latestTesting().ordinal(),
Expand All @@ -273,6 +275,8 @@ public void assertLatestProductionIsProduction() {
assertTrue(LATEST_PRODUCTION.isProduction());
}

// This test is disabled because all features are stable in Apache Kafka 4.2.
@Disabled
@Test
public void assertLatestIsNotProduction() {
assertFalse(MetadataVersion.latestTesting().isProduction());
Expand Down
5 changes: 3 additions & 2 deletions tests/kafkatest/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def get_version(node=None):

LATEST_STABLE_TRANSACTION_VERSION = 2
# This should match the LATEST_PRODUCTION version defined in MetadataVersion.java
LATEST_STABLE_METADATA_VERSION = "4.1-IV1"
LATEST_STABLE_METADATA_VERSION = "4.2-IV1"

# 2.1.x versions
V_2_1_0 = KafkaVersion("2.1.0")
Expand Down Expand Up @@ -248,4 +248,5 @@ def get_version(node=None):

# 4.2.x version
V_4_2_0 = KafkaVersion("4.2.0")
LATEST_4_2 = V_4_2_0
V_4_2_1 = KafkaVersion("4.2.1")
LATEST_4_2 = V_4_2_1
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this change yet? As a matter of fact, not even sure why the whole section for 4.2 already exists? We only add released versions here (and 4.2.0 is not release yet, and should only be added in trunk after 4.2.0 was release...) Sure, it doesn't hurt to add it, but it's dead code.

Copy link
Member

Choose a reason for hiding this comment

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

This code was added by #20471 to prevent the use of new configurations in order kafka tools, we could remove the entire 4.2 section by reversing the version check condition. self >= V_4_2_0 -> self <= LATEST_4_1

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks for clarifying. Guess, for 4.2 branch we don't really use LATEST_4_2 anywhere else, so it might be fine. And the test code you refer to uses V_4_2_0 explicitly.

Bottom line: I understand why we needed to add V_4_2_0, but both LATEST_4_2 and V_4_2_1 seem not to be necessary for 4.2 branch, and especially pointing LATEST_4_2 to an unreleased version seems odd (even if it does not break anything as it's unused).

Copy link
Member Author

Choose a reason for hiding this comment

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

I realise now that I was getting confused between Kafka versions and metadata versions.