Skip to content

KAFKA-17050: Revert group.version from 3.8#16478

Merged
jolshan merged 4 commits intoapache:3.8from
jolshan:kafka-17050-3.8
Jun 28, 2024
Merged

KAFKA-17050: Revert group.version from 3.8#16478
jolshan merged 4 commits intoapache:3.8from
jolshan:kafka-17050-3.8

Conversation

@jolshan
Copy link
Copy Markdown
Member

@jolshan jolshan commented Jun 27, 2024

Reverting due to complications when trying to fix KAFKA-17011 in 3.8. Now there will be no production features, so we won't send any over the wire in ApiVersions or BrokerRegistration and cause issues when the receiver is on an old version.

I reverted the typo PR to make the reverts cleaner and minimize chances for errors. The only conflicts were due to imports and a modified test testConsumerGroupDescribe. The fix was to keep the modified parameters but remove the metadataCache code.

@jolshan
Copy link
Copy Markdown
Member Author

jolshan commented Jun 27, 2024

I will run the system tests with this solution as well. 👍

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Jun 27, 2024

I looked at testConsumerGroupDescribe and the modified version looks fine to me. If the rest was a clean revert, I can give this a +1.

(I trust you on the import statement conflicts) :)

One question, though, we are not setting raftSupport in a bunch of calls to createKafkaApis any more. I assume that's not a problem, but it doesn't lok quite correct either (since we're then getting the ZK version of everything.) I suppose if the test doesn't hit any problems we don't need to change it in 3.8 (and it will be fixed in 3.9 and beyond)

@jolshan
Copy link
Copy Markdown
Member Author

jolshan commented Jun 27, 2024

@cmccabe Yeah, David and I had a discussion about the raftSupport. Until his change, we were always using the zk metadata cache. However, we are removing the metadata cache from those tests so I don't think it is a big deal. Or I guess to be clear -- we will want to eventually convert to all raft support, but reverting this now leaves us no worse off than before.

some info: #16120 (comment)

Copy link
Copy Markdown
Contributor

@cmccabe cmccabe left a comment

Choose a reason for hiding this comment

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

LGTM when tests pass

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@jolshan thanks for this fix. only one small comment is left. Also, I'm so sorry that the optimized imports cause a trouble on this PR 😢

assertEquals("3.7", IBP_3_7_IV2.shortVersion());
assertEquals("3.7", IBP_3_7_IV3.shortVersion());
assertEquals("3.7", IBP_3_7_IV4.shortVersion());
assertEquals("3.8", IBP_3_8_IV0.shortVersion());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess that we can keep this assertEquals("3.8", IBP_3_8_IV0.shortVersion());?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm. It's strange that this was missing before. I can add it back.

Copy link
Copy Markdown
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

LGTM

@jlprat jlprat added the Blocker This pull request is identified as solving a blocker for a release. label Jun 28, 2024
@dajac
Copy link
Copy Markdown
Member

dajac commented Jun 28, 2024

@jolshan Are we going to hit the same issue with test.feature.version?

@jolshan
Copy link
Copy Markdown
Member Author

jolshan commented Jun 28, 2024

@dajac It is not a production feature so it is not supported by the brokers/controllers.

@jolshan
Copy link
Copy Markdown
Member Author

jolshan commented Jun 28, 2024

Kraft upgrade system tests passed. Jenkins failures are unrelated. I will merge.

@jolshan
Copy link
Copy Markdown
Member Author

jolshan commented Jun 28, 2024

Just kidding -- I missed @chia7712's comment.

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@jolshan : Thanks for the PR. Left a few comments.

}

override def generateConfigs: Seq[KafkaConfig] = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extra newline

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This wasn't done by me, it was something that David removed in his PR and I reverted it. But sure we can remove it.

setBrokerId(brokerId).
setClusterId(active.clusterId()).
setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_4_0_IV0)).
setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_3_7_IV0)).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, should this be IBP_3_8_IV0?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sure

expectedFeatures.put(feature.featureName(), VersionRange.of(
0,
feature.defaultValue(MetadataVersion.latestTesting())
feature.defaultValue(MetadataVersion.LATEST_PRODUCTION)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, why is this changed to LATEST_PRODUCTION? The test is about unstable features.

@jolshan
Copy link
Copy Markdown
Member Author

jolshan commented Jun 28, 2024

My goal was to unblock the 3.8 release quickly so I didn't review every line of code I reverted. I can fix these things though.

Copy link
Copy Markdown
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

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

@jolshan
Copy link
Copy Markdown
Member Author

jolshan commented Jun 28, 2024

Nice -- green build for java 8! And the 3 failures for java 21 were unrelated. Merging now. Thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Blocker This pull request is identified as solving a blocker for a release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants