Revert "KAFKA-16154" and mark MV 3.8-IV0 as the latest production by jlprat · Pull Request #16400 · apache/kafka

jlprat · 2024-06-20T08:03:10Z

This reverts commit 55a6d30
As described in #16347 (comment)

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

jlprat · 2024-06-20T08:04:08Z

@cmccabe If you have time, this should be the revert of commit 55a6d30 on the 3.8 branch.
A generated revert was not possible as there were merge conflicts.

jlprat · 2024-06-20T08:05:35Z

-      } else if (targetTimestamp == ListOffsetsRequest.LATEST_TIERED_TIMESTAMP) {
-        if (remoteLogEnabled()) {
-          val curHighestRemoteOffset = highestOffsetInRemoteStorage()
-
-          val epochResult: Optional[Integer] =
-            if (leaderEpochCache.isDefined) {
-              val epochOpt = leaderEpochCache.get.epochForOffset(curHighestRemoteOffset)
-              if (epochOpt.isPresent) {
-                Optional.of(epochOpt.getAsInt)
-              } else if (curHighestRemoteOffset == -1) {
-                Optional.of(RecordBatch.NO_PARTITION_LEADER_EPOCH)
-              } else {
-                Optional.empty()
-              }
-            } else {
-              Optional.empty()
-            }
-
-          Some(new TimestampAndOffset(RecordBatch.NO_TIMESTAMP, curHighestRemoteOffset, epochResult))
-        } else {
-          Some(new TimestampAndOffset(RecordBatch.NO_TIMESTAMP, -1L, Optional.of(-1)))
-        }


These and the 8 lines prior were the ones with a conflict when reverting.

jlprat · 2024-06-20T12:59:34Z

Build failure contained:

14:54:56 hudson.remoting.Channel$OrderlyShutdown: Command Close created at ...

I triggered another build

junrao

@jlprat : Thanks for the PR. Added a few comments.

junrao · 2024-06-20T22:22:55Z

     * IT CANNOT BE CHANGED.</strong>
     */
-    public static final MetadataVersion LATEST_PRODUCTION = IBP_3_7_IV4;
+    public static final MetadataVersion LATEST_PRODUCTION = IBP_3_8_IV0;


3.8_IV0 is no longer controlling ELR. We need to change the following comment according to https://github.com/apache/kafka/pull/16347/files#diff-abe728e9704c4c6709c4c3f6fd3908d51219cda55791ac7f103e6042c1d6c298.

// Add ELR related supports (KIP-966). IBP_3_8_IV0(20, "3.8", "IV0", true),

Also, quite a few tests (e.g. PartitionChangeBuilderTest.java) still reference 3.8_IV0 with the assumption that it's associated with ELR. @cmccabe: Should we bring IBP_3_9_IV0, the new MV associated with ELR to the 3.8 branch or change all those tests related to ELR?

There are a few tests referring to IBP_3_7_IV4, the previous latest production version. They all need to be changed to the new latest production version IBP_3_8_IV0.

ApiVersionsRequestTest.scala

FeatureCommandTest.java

We need to add a comment above the following line that 3.8.0 is the latest production version in 3.8 line.

https://github.com/apache/kafka/blob/3.8/server-common/src/test/java/org/apache/kafka/server/common/MetadataVersionTest.java#L187

Applied feedback, except for "Should we bring IBP_3_9_IV0, the new MV associated with ELR to the 3.8 branch or change all those tests related to ELR?". I'm waiting for @cmccabe's feedback on this one.

jlprat · 2024-06-21T12:50:35Z

+    // TEST_2 released right before MV 4.0-IV0 was released, and it depends on this metadata version
+    TEST_2(2, MetadataVersion.IBP_4_0_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_4_0_IV0.featureLevel()));


@jolshan I need help understanding this test setup. As far as I could understand, this TEST_2 feature was meant to be released in a future release, given we 3.8 is the current one, I moved it to 4.0. But I think this is not a robust test. Probably I'm missing something else.

I plan to update this in a future PR. Sorry it is not easy to understand now. If you can push it to 4.0, that would be good for now.

jlprat · 2024-06-21T12:50:56Z

        // Change expected message to reflect latest MetadataVersion (SupportedMaxVersion increases when adding a new version)
        assertEquals("Feature: metadata.version\tSupportedMinVersion: 3.0-IV1\t" +
-                "SupportedMaxVersion: 4.0-IV0\tFinalizedVersionLevel: 3.7-IV4\t", outputWithoutEpoch(features.get(1)));
+                "SupportedMaxVersion: 4.0-IV0\tFinalizedVersionLevel: 3.8-IV0\t", outputWithoutEpoch(features.get(1)));


This needed to change as well as we changed the parameter of the test to 3.8-IV0

jlprat · 2024-06-26T11:07:33Z

Build failed with an exception on "streams:upgrade-system-tests-34:test". Rerunning the build

junrao · 2024-06-28T23:22:49Z

@jlprat : The PR now has conflicts after #16478. Could you rebase? Thanks.

apache#15213)" This reverts commit 55a6d30

Signed-off-by: Josep Prat <josep.prat@aiven.io>

jlprat · 2024-06-30T15:40:49Z

@junrao Rebased
@jolshan I would highly appreciate your review for "marking IBP_3_8_V0 as latest production". There are couple of changes I needed to do to make the tests pass.

jlprat · 2024-07-01T15:05:15Z

Something seems wrong after rebasing. Build hangs and the job gets terminated.
Apparently ReplicationQuotasTest > shouldBootstrapTwoBrokersWithLeaderThrottle(String) > "shouldBootstrapTwoBrokersWithLeaderThrottle(String).quorum=kraft" never terminates.

junrao

@jlprat : Thanks for the updated PR. Left a comment.

junrao · 2024-07-01T16:23:44Z

+    // Please move this comment when updating the LATEST_PRODUCTION constant.
+    //
+
+    // Introduce version 1 of the GroupVersion feature (KIP-848).


Hmm, we removed GroupVersion in 3.8 #16478. Why are we adding it back? The hanging ReplicationQuotasTest is related to this since registerBroker() needs to set MaxSupportedVersion to the latest MV, which is 4.0 now. See a similar change in trunk https://github.com/apache/kafka/pull/16347/files#diff-4f8200cb972ea4c0d08bcfc45e9e4e6295c37a4414b9acd59cc4bb248bc25a1c

Good pointer, I think this might be the problem. This was probably a wrong conflict resolution during rebasing.
Why this existed and was used in this PR is because due to the test setup for FeaturesTest.
TEST_2 needs to be configured to 4.0 like this https://github.com/apache/kafka/pull/16400/files#diff-10fd2b01213aa0cc1f879b06eb7eaae357e3dbcf8a270660b0eb4fcc78e7a5a1R27
Otherwise these tests fail:

:server-common:test > Gradle Test Executor 4 > FeaturesTest > testDefaultTestVersion(MetadataVersion) > "testDefaultTestVersion(MetadataVersion).metadataVersion=3.8-IV0" FAILED org.opentest4j.AssertionFailedError: expected: <1> but was: <2> at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151) at app//org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132) at app//org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197) at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:134) at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:129) at app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:329) at app//org.apache.kafka.server.common.FeaturesTest.testDefaultTestVersion(FeaturesTest.java:120)

Gradle Test Run :server-common:test > Gradle Test Executor 4 > FeaturesTest > testUnstableTestVersion() FAILED org.opentest4j.AssertionFailedError: Expected java.lang.IllegalArgumentException to be thrown, but nothing was thrown. at app//org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152) at app//org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:73) at app//org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:35) at app//org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3115) at app//org.apache.kafka.server.common.FeaturesTest.testUnstableTestVersion(FeaturesTest.java:125)

I'll remove it, and try to find out how to fix the tests that seem to expect MV 3.8-IV0 to not be the production one.

I think if you need an unstable version, you can just use 4.0 (or 3.9)

@jolshan Isn't this what is being done in this PR? So if I add the 4.0, the other tests work, but then ReplicationQuotasTest hangs.
This area is not one that I'm really familiar with.

Hmm. I'm not sure I follow how the tests are related. I can pull your branch if that helps.

@jolshan https://github.com/apache/kafka/pull/16400/files#diff-10fd2b01213aa0cc1f879b06eb7eaae357e3dbcf8a270660b0eb4fcc78e7a5a1R27 needs to be configured as 4.0 (or 3.9 for that matter) so the FeaturesTest works. But if I add 4.0 in MetadaVersion then ReplicationQuotasTest.shouldBootstrapTwoBrokersWithLeaderThrottle hangs.

I think I found a way. I'll push shortly

It's ok to introduce an unstable 4.0 MV for the TestFeature. We just need to (1) make sure the comment associated with the new MV is correct; (2) change all tests that depend on the latest testing MV. In #16347, we tried to change all relevant tests to use MetadataVersion.latestTesting() so that we don't have to manually change the MV when new unstable MV is added in the future. You can search for all tests that have changed from MetadataVersion.IBP_4_0_IV0 to MetadataVersion.latestTesting() in #16347 and apply them here.

I did this now for all the changes I belive

junrao · 2024-07-01T16:55:02Z

Since this PR also marks MV 3.8-IV0 as the latest production, it would be useful to update the description of the PR accordingly.

jlprat · 2024-07-01T17:32:07Z

      .setName(MetadataVersion.FEATURE_NAME)
      .setMinSupportedVersion(MetadataVersion.IBP_3_0_IV1.featureLevel())
-      .setMaxSupportedVersion(MetadataVersion.IBP_3_8_IV0.featureLevel()))
+      .setMaxSupportedVersion(MetadataVersion.latestTesting().featureLevel()))


This makes the test pass as it disregards the latest version as per #16347

jlprat · 2024-07-01T17:32:23Z

            controlEnv.activeController().registerBroker(ANONYMOUS_CONTEXT,
                new BrokerRegistrationRequestData().
-                setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_4_0_IV0)).
+                setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.latestTesting())).


As per #16347

jlprat · 2024-07-01T17:33:01Z

-
-    // Introduce version 1 of the GroupVersion feature (KIP-848).
-    IBP_4_0_IV0(21, "4.0", "IV0", false);
+    IBP_3_9_IV0(21, "3.9", "IV0", false);


Taking changes from #16347

junrao

@jlprat : Thanks for the updated PR. Left one more comment.

junrao · 2024-07-01T18:06:04Z

+    // If users attempt to use an unstable MetadataVersion, they will get an error.
+    // Please move this comment when updating the LATEST_PRODUCTION constant.
+    //
+    IBP_3_9_IV0(21, "3.9", "IV0", false);


The following comment needs to be moved to IBP_3_9_IV0.

// Add ELR related supports (KIP-966).

We need to change isElrSupported() to use IBP_3_9_IV0.

We need to bring all tests referencing IBP_3_9_IV1 for ELR in KAFKA-16968: Make 3.8-IV0 a stable MetadataVersion and create 3.9-IV0 #16347 here.

We need to bring in the changes in the following tests in KAFKA-16968: Make 3.8-IV0 a stable MetadataVersion and create 3.9-IV0 #16347 for testing latestTesting MV.
ClusterTestExtensionsTest.java
ZkMigrationIntegrationTest.scala
ApiVersionsRequestTest.scala

We need to add IBP_3_9_IV0 to all tests in MetadataVersionTest.java where 4.0-IV0 is used in trunk.

Thanks @junrao
Would we need to add IBP_3_9_IV1 as well? Or just use the tests for IBP_3_9_IV1 using the IBP_3_9_IV0 enum value?

Just use the IBP_3_9_IV0 enum value for tests using IBP_3_9_IV1 in trunk since BP_3_9_IV0 is associated with ELR in 3.8.

I think I brought all necessary changes now

ELR changed metadata records.

jlprat · 2024-07-01T19:51:43Z


  @ClusterTemplate("testApiVersionsRequestIncludesUnreleasedApisTemplate")
-  @ClusterTest(types = Array(Type.KRAFT, Type.CO_KRAFT), serverProperties = Array(
+  @ClusterTest(types = Array(Type.KRAFT, Type.CO_KRAFT), metadataVersion = MetadataVersion.IBP_3_9_IV0, serverProperties = Array(


ClusterTest in trunk has IBP_3_9_IV0 as default value instead of IBP_3_8_IV0. This took me a while to detect as I didn't understand why this test was failing.

Actually, ClusterTest.metadataVersion() should default to latest testing MV, which is IBP_3_9_IV0. Could we change that and remove setting metadataVersion in this test and the one above?

jolshan

lgtm as long as the tests look good.

junrao

@jlprat : Thanks for the updated PR. A few more comments.

junrao · 2024-07-01T20:21:31Z

+    // If users attempt to use an unstable MetadataVersion, they will get an error.
+    // Please move this comment when updating the LATEST_PRODUCTION constant.
+    //
+    IBP_3_9_IV0(21, "3.9", "IV0", false);


ELR changed metadata records.

junrao · 2024-07-01T20:34:57Z

                    setClusterId(active.clusterId()).
                    setIncarnationId(Uuid.fromString("kxAT73dKQsitIedpiPtwBA")).
-                    setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_3_8_IV0)).
+                    setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.latestTesting())).


testUncleanShutdownBroker() is based on ELR. So we need to change the 4 instances of IBP_3_8_IV0 in it to IBP_3_9_IV0. See #16347

Indeed, this test was failing. I missed this change.

junrao · 2024-07-01T21:08:50Z


  @ClusterTemplate("testApiVersionsRequestIncludesUnreleasedApisTemplate")
-  @ClusterTest(types = Array(Type.KRAFT, Type.CO_KRAFT), serverProperties = Array(
+  @ClusterTest(types = Array(Type.KRAFT, Type.CO_KRAFT), metadataVersion = MetadataVersion.IBP_3_9_IV0, serverProperties = Array(


Actually, ClusterTest.metadataVersion() should default to latest testing MV, which is IBP_3_9_IV0. Could we change that and remove setting metadataVersion in this test and the one above?

cmccabe · 2024-07-01T23:18:56Z

To respond to some of the questions above, overall,

3.8-IV0 in the 3.8 branch needs to be correct and completely in line with trunk
3.9-IV0 and 3.9-IV1 in the 3.8 branch can be whatever (it doesn't matter, since they are not usable / stable in 3.8)
As you've probably noticed, the tests which use @ClusterTest often need to be manually updated when the latest MV changes. I do think this is a problem (with @ClusterTest). I don't have any quick solution right now other than doing the edits. This isn't an issue with the tests using TestKit directly, since they can just set MetadataVersion.latestTesting. Maybe we need to rethink the @ClusterTest stuff.

Let me know if you need me specifically to review

jlprat · 2024-07-02T09:23:23Z

Thanks for the review @junrao. Feedback is now applied

jlprat · 2024-07-02T13:54:46Z

Failing tests shouldn't be related to the changes (all tests passed locally)

junrao

@jlprat : Thanks for the updated PR. LGTM

jolshan

Reapproving with the latest changes! Thanks Josep.

junrao · 2024-07-02T15:50:53Z

                    setClusterId(active.clusterId()).
                    setIncarnationId(Uuid.fromString("kxAT73dKQsitIedpiPtwBA")).
-                    setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_3_8_IV0)).
+                    setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_3_9_IV0)).


Note that ideally, we should use MetadataVersion.latestTesting() here. However, explicitly using IBP_3_9_IV0 works too since it's the latest MV.

jlprat · 2024-07-02T17:22:30Z

Merged! Thanks for the reviews!

LATEST_PRODUCTION version in MetadataVersion.java was updated in both apache#16347 and apache#16400, but it was left unchanged in the system tests.

…16533) LATEST_PRODUCTION version in MetadataVersion.java was updated in both #16347 and #16400, but it was left unchanged in the system tests. Reviewers: Josep Prat <josep.prat@aiven.io>

…pache#16533) LATEST_PRODUCTION version in MetadataVersion.java was updated in both apache#16347 and apache#16400, but it was left unchanged in the system tests. Reviewers: Josep Prat <josep.prat@aiven.io>

jlprat commented Jun 20, 2024

View reviewed changes

jlprat mentioned this pull request Jun 20, 2024

KAFKA-16968: Make 3.8-IV0 a stable MetadataVersion and create 3.9-IV0 #16347

Merged

junrao reviewed Jun 20, 2024

View reviewed changes

jlprat commented Jun 21, 2024

View reviewed changes

jlprat added the Blocker This pull request is identified as solving a blocker for a release. label Jun 24, 2024

jlprat added 6 commits June 30, 2024 15:23

Revert "KAFKA-16154: Broker returns offset for LATEST_TIERED_TIMESTAMP (

0093db8

apache#15213)" This reverts commit 55a6d30

Change LATEST_PRODUCTION to 3.8

890b2b5

Signed-off-by: Josep Prat <josep.prat@aiven.io>

Set 3.8_IV0 as latest production version in 3.8

a14e15a

Signed-off-by: Josep Prat <josep.prat@aiven.io>

Fix the tests and right value for 3.8-IV0 metadata changes

cc8efbe

Signed-off-by: Josep Prat <josep.prat@aiven.io>

Fix test

6ebecb5

Fix tests regarding metadata versions

87d0761

jlprat force-pushed the MINOR-revert-KAFKA-16154 branch from 0058068 to 87d0761 Compare June 30, 2024 15:39

junrao reviewed Jul 1, 2024

View reviewed changes

jlprat changed the title ~~Revert "KAFKA-16154: Broker returns offset for LATEST_TIERED_TIMESTAM…~~ Revert "KAFKA-16154" and mark MV 3.8-IV0 as the latest production Jul 1, 2024

Bring changes committed in KAFKA-16968

d715832

jolshan reviewed Jul 1, 2024

View reviewed changes

Comment thread server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java Outdated

jlprat commented Jul 1, 2024

View reviewed changes

Metadata didn't change for 3.8

73ec639

junrao reviewed Jul 1, 2024

View reviewed changes

Bring more changes from KAFKA-16968

990e443

jlprat commented Jul 1, 2024

View reviewed changes

Remove wrong comment from 3.8 version

1803c02

jolshan approved these changes Jul 1, 2024

View reviewed changes

junrao reviewed Jul 1, 2024

View reviewed changes

Make ClusterTest annotation metadata default to 3.9

fd44ece

junrao approved these changes Jul 2, 2024

View reviewed changes

jolshan approved these changes Jul 2, 2024

View reviewed changes

junrao reviewed Jul 2, 2024

View reviewed changes

jlprat merged commit b20a735 into apache:3.8 Jul 2, 2024

jlprat deleted the MINOR-revert-KAFKA-16154 branch July 2, 2024 17:22

soarez mentioned this pull request Jul 5, 2024

KAFKA-17083: Update LATEST_STABLE_METADATA_VERSION in system tests #16533

Merged

		// TEST_2 released right before MV 4.0-IV0 was released, and it depends on this metadata version
		TEST_2(2, MetadataVersion.IBP_4_0_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_4_0_IV0.featureLevel()));

Conversation

jlprat commented Jun 20, 2024

Committer Checklist (excluded from commit message)

Uh oh!

jlprat commented Jun 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlprat commented Jun 20, 2024

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlprat commented Jun 26, 2024

Uh oh!

junrao commented Jun 28, 2024

Uh oh!

jlprat commented Jun 30, 2024

Uh oh!

jlprat commented Jul 1, 2024

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlprat Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao commented Jul 1, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jlprat Jul 1, 2024 •

edited

Loading

cmccabe commented Jul 1, 2024 •

edited

Loading