KAFKA-16192: Introduce transaction.version and usage of flexible records to coordinators by jolshan · Pull Request #16183 · apache/kafka

jolshan · 2024-06-03T23:11:35Z

This change includes adding transaction.version (part of KIP-1022)

New transaction version 1 is introduced to support writing flexible fields in transaction state log messages.

Transaction version 2 is created in anticipation for further KIP-890 changes.

Neither are made production ready. Tests for the new transaction version and new MV are created.

Committer Checklist (excluded from commit message)

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

artemlivshits · 2024-06-06T00:24:51Z

+    IBP_4_0_IVO(21, "4.0", "IV0", false),
+
+    // Introduce version 1 of the TransactionVersion feature (KIP-890).
+    IBP_4_0_IV1(22, "4.0", "IV1", false);


Do we need a separate MV version? We could bind TV to IBP_4_0_IVO. It's still in the future, so this should be ok as far as the bootstrap is concerned.

I don't think we need it if we plan to make them both production ready at the same time

I think for now the current plan is to release both in 4.0, we could reflect this in the code (if the plans change we could change the code accordingly).

Sorry I misunderstood your question. I thought you were referring to the TVs being the same. I think we should keep separate from group version for now since I assume these will be released at different times. But don't feel super strongly.

Sounds good.

dajac · 2024-06-13T12:52:30Z

+    // Version 1 enables flexible transactional state records. (KIP-890)
+    TV_1(1, MetadataVersion.IBP_4_0_IV1, Collections.emptyMap()),


I wonder why we need a version to enable flexible records. Couldn't we use the version supporting it when we enable version 2? I may have missed something.

This was a discussion I had with @cmccabe. I think the idea was that even if we want to disable transaction version, we may still want to keep our flexible records due to the compatibility reasons. I know Jeff worked on that KIP to ignore the fields, but this was an extra safeguard.

Understood. As you said, this is not strictly necessary but it does not hurt.

artemlivshits

LGTM

artemlivshits · 2024-06-13T20:38:17Z

+    IBP_4_0_IVO(21, "4.0", "IV0", false),
+
+    // Introduce version 1 of the TransactionVersion feature (KIP-890).
+    IBP_4_0_IV1(22, "4.0", "IV1", false);


Sounds good.

dajac · 2024-06-14T09:00:15Z

    // generate the message for this transaction metadata
    val keyBytes = TransactionLog.keyToBytes(transactionalId)
-    val valueBytes = TransactionLog.valueToBytes(newMetadata)
+    val valueBytes = TransactionLog.valueToBytes(newMetadata, usesFlexibleRecords())


Should we add a test to ensure that this new logic works as expected?

Functionally there is no difference now, so a test didn't come to my mind easily. I guess the best thing is to just look at the version of the record generated. I can add that.

dajac · 2024-06-14T09:01:45Z

+    // Version 1 enables flexible transactional state records. (KIP-890)
+    TV_1(1, MetadataVersion.IBP_4_0_IV1, Collections.emptyMap()),


Understood. As you said, this is not strictly necessary but it does not hurt.

dajac

LGTM, thanks. We may want to merge it after #16347 though.

jolshan · 2024-06-19T17:27:59Z

Ack -- I suppose so. I was hoping it would be merged by today 😅

jolshan · 2024-06-19T17:29:09Z

In the meantime I will fix the test failure. Whoops.

jolshan · 2024-07-10T23:19:20Z

Yeehaw time to rebase

jolshan · 2024-07-10T23:21:37Z

I think I also need to fix the case where max version is 0 as well. 🤔

junrao

@jolshan : Thanks for the PR. Left a minor comment.

junrao · 2024-07-10T23:19:01Z

    // Add ELR related supports (KIP-966).
    IBP_3_9_IV1(22, "3.9", "IV1", true),

+    IBP_3_9_IV2(23, "3.9", "IV2", false),


Could we add a comment that this is for bootstrapping TV?

Sounds good. I also need to make a few other changes when I rebase, so be on the lookout for more soon :)

I planned to make the update where we don't send features as part of the registration/api versions if the version range is 0-0. I need to do this because Colin's fix only changes the min version. It is safe because a version range 0-0 is essentially the same as not supporting the feature. We will have it here until the MV is marked as production ready.

I planned to make the update where we don't send features as part of the registration/api versions if the version range is 0-0.

Is that for the old versions of the requests or both the old and the latest version?

I think we can just do it regardless of the version of the request. I was planning to do it in Quorum/BrokerFeatures files, which would just be the controller/broker's own view of supported features. Alternatively I can do it in the request.

cc: @cmccabe

Looks like Colin already pushed this as part of the kraft.version PR. I will just resolve the conflicts and update the comment you mention.

Should we add a new MV just for bootstrapping a new feature or just piggyback on an existing one (e.g., IBP_3_9_IV1)?

I planned to make the update where we don't send features as part of the registration/api versions if the version range is 0-0.

Colin's PR only skipped a finalized version 0 feature in ApiResponse. Do you plan to do that for supported features for registration/api? Range 0-0 can now happen since some of the v1 features are not stable yet.

Hmmm-- I thought Colin's PR would cover what I need to cover since I didn't think his MV was stable yet either

I've updated the PR to include the change for broker registration

…192-1

junrao

@jolshan : Thanks for the updated PR. Left another comment.

junrao · 2024-07-18T21:17:05Z

    // Add ELR related supports (KIP-966).
    IBP_3_9_IV1(22, "3.9", "IV1", true),

+    IBP_3_9_IV2(23, "3.9", "IV2", false),


Should we add a new MV just for bootstrapping a new feature or just piggyback on an existing one (e.g., IBP_3_9_IV1)?

I planned to make the update where we don't send features as part of the registration/api versions if the version range is 0-0.

Colin's PR only skipped a finalized version 0 feature in ApiResponse. Do you plan to do that for supported features for registration/api? Range 0-0 can now happen since some of the v1 features are not stable yet.

…eature is 0-0.

jolshan · 2024-07-19T16:29:11Z

Will look at the apiversions test failures 👍

junrao

@jolshan : Thanks for the updated PR. A few more comments. Also, there seem to be quite a few test failures (ApiVersionsRequestTest, FeaturesTest) related to this PR?

junrao · 2024-07-19T16:34:05Z

    _supportedFeatures.asScala.foreach {
-      case (name, range) => features.add(new BrokerRegistrationRequestData.Feature().
+      // Do not include features with the range 0-0.
+      case (name, range) if range.max() > 0 => features.add(new BrokerRegistrationRequestData.Feature().


We should do the same for supported features in ApiVersionResponse in the following code in BrokerFeatures, right? Also, to avoid duplicating the code, perhaps it's better to do the filtering logic in BrokerFeatures?

def defaultSupportedFeatures(unstableFeatureVersionsEnabled: Boolean): Features[SupportedVersionRange] = { val features = new util.HashMap[String, SupportedVersionRange]() features.put(MetadataVersion.FEATURE_NAME, new SupportedVersionRange( MetadataVersion.MINIMUM_KRAFT_VERSION.featureLevel(), if (unstableFeatureVersionsEnabled) { MetadataVersion.latestTesting.featureLevel } else { MetadataVersion.latestProduction.featureLevel })) PRODUCTION_FEATURES.forEach { feature => features.put(feature.featureName, new SupportedVersionRange(0, if (unstableFeatureVersionsEnabled) { feature.latestTesting } else { feature.latestProduction })) } Features.supportedFeatures(features) }

I had considered putting it in BrokerFeatures/QuorumFeatures, but decided against it.
Are you suggesting that I remove how Colin wrote the code (in the ApiVersionsResponse itself).
I was trying to avoid removing his code, but if you think it is better that way I can do it. (I will also need to move the test I wrote elsewhere)

The main thing is that we need to exclude range 0-0 for supported features in ApiVersionResponse too. Colin's code in ApiVersionsResponse doesn't seem to do what you are doing here for BrokerRegistrationRequest by excluding range 0-0 for supported features. It only changes minVersion v0 to v1 for older requests.

Yeah. It is already excluded from Colin's commit. If we want to share the logic we can move it to Broker/QuorumFeatures, but I wasn't sure if there was a reason why Colin did it in the response before.

cc: @cmccabe

From my point of view:

We should never report a 0-0 supported versions range in an RPC. There is no point! This applies to both ApiVersionsResponse and BrokerRegistrationRequest.

Obviously a 1-0 range is even worse, being incorrect as well as useless.

Having a HashMap or something that contains a 0-0 range is fine, and may simplify the code in some ways. For example if we have code that changes the maximum supported range based on a config, it would probably be annoying to have to mutate the HashMap or use a different hash map if that config was on or off.

I don't feel really strongly about the third point, but I suspect that the code will be messier if you try to remove features with supported version 0-0 from the internal data structures than if you don't.

My proposal was that in defaultSupportedFeatues, we iterate over the features and add them to a map. Instead of just adding them all, we check if the range is 0-0. If so, we don't add them. (No removal needed) Thus, we don't need specific logic in the apis to remove the feature later.

The only concern is if somehow the features map expected a feature to exist. Not sure I follow mutating if the config is on or off. I assume we can't change the config to enable unstable features dynamically. (If we did, we'd have to mutate the map anyway)

Code I refer to is:

kafka/metadata/src/main/java/org/apache/kafka/controller/QuorumFeatures.java

Line 65 in 208bb1f

for (Features feature : Features.PRODUCTION_FEATURES) {

kafka/core/src/main/scala/kafka/server/BrokerFeatures.scala

Line 93 in 208bb1f

PRODUCTION_FEATURES.forEach { feature => features.put(feature.featureName,

Pushed code with this example.

@jolshan : If you think it's easier to just omit them from the map, that's fine.

I still think we should be checking that these things don't exist later on (and for example, throw an exception if they do) to avoid regressions if someone changes something (as has been happening a lot lately. :) )

I'm happy to try to make things safer. Can you point me to an example of where you would expect to check if things exist? Do you mean like when building the request? Or something else?

jolshan · 2024-07-19T16:54:20Z

Given that Kraft version starts at 0, I guess we should have all versions start a zero for consistency. I also noticed one test asserts all features with version zero bootstrap with minimum kraft version.

junrao

@jolshan : Thanks for the updated PR. Just a few more minor comments.

junrao

@jolshan : Thanks for the updated PR. It seems there is compilation failure.

junrao · 2024-07-23T16:18:06Z

@jolshan : Thanks for the updated PR. The following test failure seems related to the PR.

[2024-07-23T02:46:53.731Z] 
[2024-07-23T02:46:53.731Z] Gradle Test Run :metadata:test > Gradle Test Executor 88 > QuorumFeaturesTest > testDefaultFeatureMap() STARTED
[2024-07-23T02:46:53.731Z] org.apache.kafka.controller.QuorumFeaturesTest.testDefaultFeatureMap() failed, log available in /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-16183/metadata/build/reports/testOutput/org.apache.kafka.controller.QuorumFeaturesTest.testDefaultFeatureMap().test.stdout
[2024-07-23T02:46:53.731Z] 
[2024-07-23T02:46:53.731Z] Gradle Test Run :metadata:test > Gradle Test Executor 88 > QuorumFeaturesTest > testDefaultFeatureMap() FAILED
[2024-07-23T02:46:53.731Z]     org.opentest4j.AssertionFailedError: expected: <{kraft.version=0, transaction.version=0, metadata.version=1-20}> but was: <{metadata.version=1-20}>
[2024-07-23T02:46:53.731Z]         at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
[2024-07-23T02:46:53.731Z]         at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
[2024-07-23T02:46:53.731Z]         at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
[2024-07-23T02:46:53.731Z]         at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
[2024-07-23T02:46:53.731Z]         at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
[2024-07-23T02:46:53.731Z]         at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1145)
[2024-07-23T02:46:53.731Z]         at org.apache.kafka.controller.QuorumFeaturesTest.testDefaultFeatureMap(QuorumFeaturesTest.java:69)

junrao

@jolshan : Thanks for the updated PR. Added one more comment.

junrao · 2024-07-23T19:56:24Z

+                expectedFeatures.put(feature.featureName(), VersionRange.of(
+                    feature.minimumProduction(),
+                    maxVersion
+                ));


The code in the test is pretty much the same as the actual code. Does this and the next test add much value?

I would say maybe if someone changes in the file, we would catch that this is desired behavior and the test would fail?

I can add a comment if that helps.

junrao

@jolshan : Thanks for explanation. The PR LGTM assuming the tests pass.

…192-1

jolshan · 2024-07-24T22:34:52Z

Some of the gradle task executors failed so I will run again.

…rds to coordinators (apache#16183) This change includes adding transaction.version (part of KIP-1022) New transaction version 1 is introduced to support writing flexible fields in transaction state log messages. Transaction version 2 is created in anticipation for further KIP-890 changes. Neither are made production ready. Tests for the new transaction version and new MV are created. Also include change to not report a feature as supported if the range is 0-0. Reviewers: Jun Rao <junrao@apache.org>, David Jacot <djacot@confluent.io>, Artem Livshits <alivshits@confluent.io>, Colin P. McCabe <cmccabe@apache.org>

Add transaction version and support for flexible format

98c8d15

jolshan changed the title ~~KAFKA-16192: Introduce usage of flexible records to coordinators~~ KAFKA-16192: Introduce transaction.version and usage of flexible records to coordinators Jun 3, 2024

artemlivshits reviewed Jun 6, 2024

View reviewed changes

jolshan added 2 commits June 7, 2024 15:55

Merge branch 'trunk' of github.com:apache/kafka into kafka-16192-1

6b3ace4

Add comment and clarification

1550222

dajac reviewed Jun 13, 2024

View reviewed changes

artemlivshits approved these changes Jun 13, 2024

View reviewed changes

dajac reviewed Jun 14, 2024

View reviewed changes

Fixes and tests

541b49e

dajac approved these changes Jun 19, 2024

View reviewed changes

jolshan added 5 commits June 19, 2024 10:46

Fix test

249917d

Fix imports

c12ca5f

Merge branch 'trunk' of github.com:apache/kafka into kafka-16192-1

32b1bbd

test fixes

d2ae555

Fix metadata version values

7b3fc9c

jolshan mentioned this pull request Jul 3, 2024

KAFKA-17011: Fix a bug preventing features from supporting v0 #16421

Merged

junrao reviewed Jul 10, 2024

View reviewed changes

Merge branch 'trunk' of https://github.com/apache/kafka into kafka-16…

bde2d4e

…192-1

junrao reviewed Jul 18, 2024

View reviewed changes

Update to not include a feature in registration when the range of a f…

fada954

…eature is 0-0.

jolshan force-pushed the kafka-16192-1 branch from afc2d68 to fada954 Compare July 18, 2024 23:57

junrao reviewed Jul 19, 2024

View reviewed changes

Change logic for supported features

dc34d93

junrao reviewed Jul 19, 2024

View reviewed changes

Addressing comments

432fe47

junrao reviewed Jul 20, 2024

View reviewed changes

Fix spotless issue

87ca44d

fix test

0598cb8

junrao reviewed Jul 23, 2024

View reviewed changes

junrao approved these changes Jul 23, 2024

View reviewed changes

Merge branch 'trunk' of https://github.com/apache/kafka into kafka-16…

22af8c7

…192-1

jolshan merged commit a0f6e6f into apache:trunk Jul 26, 2024

		// Version 1 enables flexible transactional state records. (KIP-890)
		TV_1(1, MetadataVersion.IBP_4_0_IV1, Collections.emptyMap()),

Conversation

jolshan commented Jun 3, 2024

Committer Checklist (excluded from commit message)

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!

jolshan Jun 7, 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!

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!

Uh oh!

artemlivshits 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!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

jolshan commented Jun 19, 2024

Uh oh!

jolshan commented Jun 19, 2024

Uh oh!

jolshan commented Jul 10, 2024

Uh oh!

jolshan commented Jul 10, 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolshan commented Jul 19, 2024

Uh oh!

junrao left a comment

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

jolshan Jun 7, 2024 •

edited

Loading

junrao Jul 19, 2024 •

edited

Loading

cmccabe Jul 19, 2024 •

edited

Loading

jolshan Jul 19, 2024 •

edited

Loading