KAFKA-18052: Decouple the dependency of feature stable version to the metadata version#17886
Conversation
dongnuo123
changed the title
dongnuo123
changed the title
dongnuo123
changed the title
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@dongnuo123 : Thanks for the PR. Left a comment.
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@dongnuo123 : Thanks for the updated PR. A few minor comments.
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@dongnuo123 : Thanks for the updated PR. A few more comments.
| UNIT_TEST_VERSION_2(UnitTestFeatureVersion.FEATURE_NAME + ".2", new FeatureVersion[]{UT_FV2_0, UT_FV2_1}, UT_FV2_0), | ||
| UNIT_TEST_VERSION_3(UnitTestFeatureVersion.FEATURE_NAME + ".3", new FeatureVersion[]{UT_FV3_0, UT_FV3_1}, UT_FV3_1), | ||
| UNIT_TEST_VERSION_4(UnitTestFeatureVersion.FEATURE_NAME + ".4", new FeatureVersion[]{UT_FV4_0, UT_FV4_1}, UT_FV4_1), | ||
| UNIT_TEST_VERSION_5(UnitTestFeatureVersion.FEATURE_NAME + ".5", new FeatureVersion[]{UT_FV5_0, UT_FV5_1}, UT_FV5_1); |
There was a problem hiding this comment.
It's kind of weird to include unit test code in production code.
There was a problem hiding this comment.
Yeah it's a bit weird. The formatter unit tests are also failing because of these unrecognized features. I've tried several ways of addressing it but still don't have a rather good solution.
The issues are that 1) Feature is an enum so we can't extend from it to create a new enum specially for unit tests 2) The validation calls featureFromName which needs the Features[] array, so the Features for unit tests have to really exist.
Another way to separate the production features and the unit test features is to create an exactly same enum called UnitTestFeatures, but that will be much duplication and pretty hard to maintain, which I don't really want to adopt
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@dongnuo123 : Thanks for the updated PR. A few more comments.
| dependencyFeature.defaultLevel(MetadataVersion.LATEST_PRODUCTION))); | ||
| } | ||
| } else { | ||
| if (dependency.getValue() > defaultVersion.bootstrapMetadataVersion().featureLevel()) { |
There was a problem hiding this comment.
defaultVersion.bootstrapMetadataVersion().featureLevel() => metadataVersion ?
There was a problem hiding this comment.
The two are not necessarily equal. For instance, when metadataVersion=120, the feature has versions
- FV_1, bootstrap MV = 119
- FV_2, bootstrap MV = 121
Then defaultVersion.bootstrapMetadataVersion() will be 119. Using metadataVersion would also work since we do check when the two are equal, but I feel comparing with the bootstrap MV makes more sense.
|
|
||
| // For testing the default value has MV dependency that is ahead of the bootstrap MV. | ||
| UT_FV7_0(0, MetadataVersion.MINIMUM_KRAFT_VERSION, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_3_7_IV0.featureLevel())), | ||
| UT_FV7_1(1, MetadataVersion.IBP_3_8_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_3_8_IV0.featureLevel())); |
There was a problem hiding this comment.
It seems that we don't really need UT_FV7_1?
There was a problem hiding this comment.
Adding UT_FV7_1 was meant to test when the failed feature version is not the default version of latest production mv, but yes there's no impact removing it
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@dongnuo123 : Thanks for the updated PR. Just a few minor comments.
junrao
left a comment
junrao
left a comment
There was a problem hiding this comment.
@dongnuo123 : Thanks for the updated PR. LGTM. The failed tests are marked as flaky. So, will merge this PR.
… metadata version (apache#17886) Currently the validation of feature upgrade relies on the supported version range generated during registration. For a given feature, its max supported feature version in production is set to be the default version value (the latest feature version with bootstrap metadata value smaller or equal to the latest production metadata value). This patch introduces a LATEST_PRODUCTION value independent from the metadata version to each feature so that the highest supported feature version can be customized by the feature owner. The change only applies to dynamic feature upgrade. During formatting, we still use the default value associated the metadata version. Reviewers: Justine Olshan <jolshan@confluent.io>, Jun Rao <junrao@gmail.com>
4 participants
Currently the validation of feature upgrade relies on the supported version range generated during registration. For a given feature, its max supported feature version in production is set to be the default version value (the latest feature version with bootstrap metadata value smaller or equal to the latest production metadata value).
This patch introduces a
LATEST_PRODUCTIONvalue independent from the metadata version to each feature so that the highest supported feature version can be customized by the feature owner.The change only applies to dynamic feature upgrade. During formatting, we still use the default value associated the metadata version.
Committer Checklist (excluded from commit message)