junrao commented on code in PR #18845:
URL: https://github.com/apache/kafka/pull/18845#discussion_r1951432926
##########
metadata/src/test/java/org/apache/kafka/controller/FeatureControlManagerTest.java:
##########
@@ -379,7 +373,7 @@ public void testCannotDowngradeBefore3_3_IV0() {
public void testCreateFeatureLevelRecords() {
Map<String, VersionRange> localSupportedFeatures = new HashMap<>();
localSupportedFeatures.put(MetadataVersion.FEATURE_NAME,
VersionRange.of(
- MetadataVersion.IBP_3_0_IV1.featureLevel(),
MetadataVersion.latestTesting().featureLevel()));
+ MetadataVersionTestUtils.IBP_3_0_IV1_FEATURE_LEVEL,
MetadataVersion.latestTesting().featureLevel()));
Review Comment:
We probably should set the min to MetadataVersion.MINIMUM_VERSION?
##########
metadata/src/test/java/org/apache/kafka/image/loader/MetadataLoaderTest.java:
##########
@@ -310,7 +310,7 @@ public void testRemovePublisher() throws Exception {
singletonList(singletonList(new ApiMessageAndVersion(
new FeatureLevelRecord().
setName(MetadataVersion.FEATURE_NAME).
- setFeatureLevel(IBP_3_3_IV2.featureLevel()), (short)
0))));
+ setFeatureLevel(IBP_3_3_IV3.featureLevel()), (short)
0))));
Review Comment:
Since this can be any MV, could we just use MetadataVersion.MINIMUM_VERSION?
##########
metadata/src/test/java/org/apache/kafka/image/MetadataVersionChangeTest.java:
##########
@@ -30,31 +30,31 @@
@Timeout(value = 40)
public class MetadataVersionChangeTest {
- private static final MetadataVersionChange CHANGE_3_0_IV1_TO_3_3_IV0 =
- new MetadataVersionChange(IBP_3_0_IV1, IBP_3_3_IV0);
+ private static final MetadataVersionChange CHANGE_3_3_IV3_TO_3_6_IV0 =
Review Comment:
We just need two different MVs here. Could we use
MetadataVersion.MINIMUM_VERSION and MetadataVersion.latestProduction?
##########
metadata/src/test/java/org/apache/kafka/controller/FeatureControlManagerTest.java:
##########
@@ -240,38 +241,29 @@ public void testReplayRecords() {
private static final FeatureControlManager.Builder TEST_MANAGER_BUILDER1 =
new FeatureControlManager.Builder().
setQuorumFeatures(features(MetadataVersion.FEATURE_NAME,
- MetadataVersion.IBP_3_3_IV0.featureLevel(),
MetadataVersion.IBP_3_3_IV3.featureLevel())).
- setMetadataVersion(MetadataVersion.IBP_3_3_IV2);
+ MetadataVersion.MINIMUM_VERSION.featureLevel(),
MetadataVersion.IBP_3_6_IV0.featureLevel())).
+ setMetadataVersion(MetadataVersion.IBP_3_4_IV0);
@Test
public void testApplyMetadataVersionChangeRecord() {
FeatureControlManager manager = TEST_MANAGER_BUILDER1.build();
manager.replay(new FeatureLevelRecord().
setName(MetadataVersion.FEATURE_NAME).
- setFeatureLevel(MetadataVersion.IBP_3_3_IV3.featureLevel()));
- assertEquals(MetadataVersion.IBP_3_3_IV3, manager.metadataVersion());
- }
-
- @Test
- public void
testCannotDowngradeToVersionBeforeMinimumSupportedKraftVersion() {
- FeatureControlManager manager = TEST_MANAGER_BUILDER1.build();
- assertEquals(ControllerResult.of(Collections.emptyList(), new
ApiError(Errors.INVALID_UPDATE_VERSION,
- "Invalid update version 3 for feature metadata.version. Local
controller 0 only " +
- "supports versions 4-7")),
- manager.updateFeatures(
- singletonMap(MetadataVersion.FEATURE_NAME,
MetadataVersion.IBP_3_2_IV0.featureLevel()),
- singletonMap(MetadataVersion.FEATURE_NAME,
FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE),
- true));
+ setFeatureLevel(MetadataVersion.IBP_3_4_IV0.featureLevel()));
Review Comment:
Should we use MetadataVersion.MINIMUM_VERSION?
##########
metadata/src/test/java/org/apache/kafka/image/loader/MetadataLoaderTest.java:
##########
@@ -349,11 +349,11 @@ public void testLoadEmptySnapshot() throws Exception {
assertEquals(300L, loader.lastAppliedOffset());
assertEquals(new SnapshotManifest(new MetadataProvenance(300, 100,
4000, true), 3000000L),
publishers.get(0).latestSnapshotManifest);
- assertEquals(MetadataVersion.MINIMUM_KRAFT_VERSION,
+ assertEquals(MetadataVersion.MINIMUM_VERSION,
loader.metrics().currentMetadataVersion());
}
assertTrue(publishers.get(0).closed);
- assertEquals(MetadataVersion.IBP_3_0_IV1,
+ assertEquals(IBP_3_3_IV3,
Review Comment:
Could we just use MetadataVersion.MINIMUM_VERSION?
##########
metadata/src/test/java/org/apache/kafka/controller/FeatureControlManagerTest.java:
##########
@@ -349,28 +344,27 @@ public void testCanUseUnsafeDowngradeIfMetadataChanged() {
public void testCanUseSafeDowngradeIfMetadataDidNotChange() {
FeatureControlManager manager = new FeatureControlManager.Builder().
setQuorumFeatures(features(MetadataVersion.FEATURE_NAME,
- MetadataVersion.IBP_3_0_IV1.featureLevel(),
MetadataVersion.IBP_3_3_IV1.featureLevel())).
- setMetadataVersion(MetadataVersion.IBP_3_1_IV0).
- setMinimumBootstrapVersion(MetadataVersion.IBP_3_0_IV1).
+ MetadataVersion.MINIMUM_VERSION.featureLevel(),
MetadataVersion.IBP_3_6_IV0.featureLevel())).
+ setMetadataVersion(MetadataVersion.IBP_3_5_IV0).
build();
assertEquals(ControllerResult.of(Collections.emptyList(),
ApiError.NONE),
manager.updateFeatures(
- singletonMap(MetadataVersion.FEATURE_NAME,
MetadataVersion.IBP_3_0_IV1.featureLevel()),
+ singletonMap(MetadataVersion.FEATURE_NAME,
MetadataVersion.IBP_3_4_IV0.featureLevel()),
singletonMap(MetadataVersion.FEATURE_NAME,
FeatureUpdate.UpgradeType.SAFE_DOWNGRADE),
true));
}
@Test
- public void testCannotDowngradeBefore3_3_IV0() {
+ public void testCannotDowngradeBeforeMinimumKraftVersion() {
FeatureControlManager manager = new FeatureControlManager.Builder().
setQuorumFeatures(features(MetadataVersion.FEATURE_NAME,
- MetadataVersion.IBP_3_0_IV1.featureLevel(),
MetadataVersion.IBP_3_3_IV3.featureLevel())).
- setMetadataVersion(MetadataVersion.IBP_3_3_IV0).
+ MetadataVersionTestUtils.IBP_3_0_IV1_FEATURE_LEVEL,
MetadataVersion.MINIMUM_VERSION.featureLevel())).
Review Comment:
We probably should set the min/max to `MetadataVersion.MINIMUM_VERSION` and
`MetadataVersion.latestTesting()`.
##########
metadata/src/test/java/org/apache/kafka/image/loader/MetadataLoaderTest.java:
##########
@@ -511,7 +511,7 @@ public void testLastAppliedOffset() throws Exception {
new MetadataProvenance(200, 100, 4000, true), asList(
singletonList(new ApiMessageAndVersion(new
FeatureLevelRecord().
setName(MetadataVersion.FEATURE_NAME).
- setFeatureLevel(IBP_3_3_IV1.featureLevel()), (short)
0)),
+ setFeatureLevel(IBP_3_3_IV3.featureLevel()), (short)
0)),
Review Comment:
Since this can be any MV, could we just use MetadataVersion.MINIMUM_VERSION?
##########
metadata/src/test/java/org/apache/kafka/image/FeaturesImageTest.java:
##########
@@ -163,7 +163,7 @@ public void testEmpty() {
assertFalse(new FeaturesImage(Collections.singletonMap("foo", (short)
1),
FeaturesImage.EMPTY.metadataVersion()).isEmpty());
assertFalse(new FeaturesImage(FeaturesImage.EMPTY.finalizedVersions(),
- MetadataVersion.IBP_3_3_IV0).isEmpty());
+ MetadataVersion.IBP_3_4_IV0).isEmpty());
Review Comment:
Since this can be any MV, could we just use MetadataVersion.MINIMUM_VERSION?
##########
metadata/src/test/java/org/apache/kafka/controller/FeatureControlManagerTest.java:
##########
@@ -240,38 +241,29 @@ public void testReplayRecords() {
private static final FeatureControlManager.Builder TEST_MANAGER_BUILDER1 =
new FeatureControlManager.Builder().
setQuorumFeatures(features(MetadataVersion.FEATURE_NAME,
- MetadataVersion.IBP_3_3_IV0.featureLevel(),
MetadataVersion.IBP_3_3_IV3.featureLevel())).
- setMetadataVersion(MetadataVersion.IBP_3_3_IV2);
+ MetadataVersion.MINIMUM_VERSION.featureLevel(),
MetadataVersion.IBP_3_6_IV0.featureLevel())).
+ setMetadataVersion(MetadataVersion.IBP_3_4_IV0);
@Test
public void testApplyMetadataVersionChangeRecord() {
FeatureControlManager manager = TEST_MANAGER_BUILDER1.build();
manager.replay(new FeatureLevelRecord().
setName(MetadataVersion.FEATURE_NAME).
- setFeatureLevel(MetadataVersion.IBP_3_3_IV3.featureLevel()));
- assertEquals(MetadataVersion.IBP_3_3_IV3, manager.metadataVersion());
- }
-
- @Test
- public void
testCannotDowngradeToVersionBeforeMinimumSupportedKraftVersion() {
- FeatureControlManager manager = TEST_MANAGER_BUILDER1.build();
- assertEquals(ControllerResult.of(Collections.emptyList(), new
ApiError(Errors.INVALID_UPDATE_VERSION,
- "Invalid update version 3 for feature metadata.version. Local
controller 0 only " +
- "supports versions 4-7")),
- manager.updateFeatures(
- singletonMap(MetadataVersion.FEATURE_NAME,
MetadataVersion.IBP_3_2_IV0.featureLevel()),
- singletonMap(MetadataVersion.FEATURE_NAME,
FeatureUpdate.UpgradeType.UNSAFE_DOWNGRADE),
- true));
+ setFeatureLevel(MetadataVersion.IBP_3_4_IV0.featureLevel()));
+ assertEquals(MetadataVersion.IBP_3_4_IV0, manager.metadataVersion());
}
@Test
public void testCannotDowngradeToHigherVersion() {
- FeatureControlManager manager = TEST_MANAGER_BUILDER1.build();
+ FeatureControlManager manager = new FeatureControlManager.Builder().
Review Comment:
Could we use TEST_MANAGER_BUILDER1? This will make this test the same as
`testCannotUnsafeDowngradeToHigherVersion` except for the unsafe flag, which is
a bit easier to understand.
##########
metadata/src/test/java/org/apache/kafka/image/loader/MetadataLoaderTest.java:
##########
@@ -597,7 +597,7 @@ private void loadTestSnapshot2(
new MetadataProvenance(offset, 100, 4000, true), asList(
singletonList(new ApiMessageAndVersion(new
FeatureLevelRecord().
setName(MetadataVersion.FEATURE_NAME).
- setFeatureLevel(IBP_3_3_IV2.featureLevel()),
(short) 0)),
+ setFeatureLevel(IBP_3_4_IV0.featureLevel()),
(short) 0)),
Review Comment:
Since this can be any MV larger than MetadataVersion.MINIMUM_VERSION, could
we just use MetadataVersion.latestProduction?
##########
server-common/src/test/java/org/apache/kafka/server/common/MetadataVersionTest.java:
##########
@@ -29,38 +29,20 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
class MetadataVersionTest {
- @Test
- public void testKRaftFeatureLevelsBefore3_0_IV1() {
- for (int i = 0; i < MetadataVersion.IBP_3_0_IV1.ordinal(); i++) {
- assertEquals(-1, MetadataVersion.VERSIONS[i].featureLevel());
- }
- }
@Test
- public void testKRaftFeatureLevelsAtAndAfter3_0_IV1() {
- for (int i = MetadataVersion.IBP_3_0_IV1.ordinal(); i <
MetadataVersion.VERSIONS.length; i++) {
- int expectedLevel = i - MetadataVersion.IBP_3_0_IV1.ordinal() + 1;
+ public void testFeatureLevels() {
+ for (int i = MINIMUM_VERSION.ordinal(); i <
MetadataVersion.VERSIONS.length; i++) {
+ int expectedLevel = i + 7;
Review Comment:
Could we use MINIMUM_VERSION.featureLevel() instead of 7?
##########
metadata/src/test/java/org/apache/kafka/image/loader/MetadataLoaderTest.java:
##########
@@ -581,7 +581,7 @@ private void loadTestSnapshot(
new MetadataProvenance(offset, 100, 4000, true), asList(
singletonList(new ApiMessageAndVersion(new
FeatureLevelRecord().
setName(MetadataVersion.FEATURE_NAME).
- setFeatureLevel(IBP_3_3_IV1.featureLevel()),
(short) 0)),
+ setFeatureLevel(IBP_3_3_IV3.featureLevel()),
(short) 0)),
Review Comment:
Since this can be any MV, could we just use MetadataVersion.MINIMUM_VERSION?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
