MINOR: Several fixes and improvements for FeatureControlManager by cmccabe · Pull Request #12207 · apache/kafka

cmccabe · 2022-05-24T23:46:15Z

This PR fixes a bug where FeatureControlManager#replay(FeatureLevelRecord) was throwing an
exception if not all controllers in the quorum supported the feature being applied. While we do
want to validate this, it needs to be validated earlier, before the record is committed to the log.
Once the record has been committed to the log it should always be applied if the current controller
supports it.

Fix another bug where removing a feature was not supported once it had been configured. Note that
because we reserve feature level 0 for "feature not enabled", we don't need to use
Optional; we can just return a range of 0-0 when the feature is not supported.

In error messages, give more detail about what the problem is when a controller in the quorum cannot
handle a new version. For example, "Local controller 0 does not support this feature" rather than just
"quorum does not support this feature".

Allow the metadata version to be downgraded when UpgradeType.UNSAFE_DOWNGRADE has been set.
Previously we were unconditionally denying this even when this was set.

Add a builder for FeatureControlManager, so that we can easily add new parameters to the
constructor in the future. This will also be useful for creating FeatureControlManagers that are
initialized to a specific MetadataVersion.

This PR fixes a bug where FeatureControlManager#replay(FeatureLevelRecord) was throwing an exception if not all controllers in the quorum supported the feature being applied. While we do want to validate this, it needs to be validated earlier, before the record is committed to the log. Once the record has been committed to the log it should always be applied if the current controller supports it. Fix another bug where removing a feature was not supported once it had been configured. Note that because we reserve feature level 0 for "feature not enabled", we don't need to use Optional<VersionRange>; we can just return a range of 0-0 when the feature is not supported. Allow the metadata version to be downgraded when UpgradeType.UNSAFE_DOWNGRADE has been set. Previously we were unconditionally denying this even when this was set. Add a builder for FeatureControlManager, so that we can easily add new parameters to the constructor in the future. This will also be useful for creating FeatureControlManagers that are initialized to a specific MetadataVersion.

mumrah

Thanks @cmccabe! Overall looks good, a few comments inline

mumrah · 2022-05-25T17:24:27Z

    }

    public void replay(FeatureLevelRecord record) {
-        if (!canSupportVersion(record.name(), record.featureLevel())) {


This made me wonder about bootstrapping and if we could load in an unknown version. However, looking at BootstrapMetadata#load, we will parse the feature level as a MetadataVersion before returning so we would throw an exception as we read the bootstrap records into memory. This could happen if someone generates a bootstrap snapshot at version N and then tries to load it into a cluster at N-1

Yeah.

It's fine to do a sanity check based on local supported versions. Pulling in the quorum is where it is incorrect (which might be an argument for pulling localSupportedFeatures out of QuorumFeatures, but one step at a time.)

mumrah · 2022-05-25T17:25:24Z

        }
    }

-    ControllerResult<Map<String, ApiError>> initializeMetadataVersion(short initVersion) {


Yea, this was leftover from a previous iteration on the bootstrapping logic where the controller was just getting the initial metadata version.

mumrah · 2022-05-25T17:32:14Z

-            return invalidMetadataVersion(newVersionLevel, "The quorum does not support metadata.version.");
-        }
-
-        if (newVersionLevel <= 0) {


Should we allow a metadata.version of zero? Seems like it might complicate things to support a non-present metadata.version at runtime. Would that mean we need to revert back to reading the IBP from config?

I was thinking about this too.

I would like to get rid of MetadataVersion.UNINITIALIZED as a concept. There are a few places where the MV isn't stored and it should be (pre-3.3 snapshots, for example) but we can just treat those as MetadataVersion.3_0_0IV1

That would allow us to just say that MV for a kraft cluster was always > 0

But this PR was already getting too big so I held off on that.

mumrah · 2022-05-25T17:33:43Z

+     * @param level         The feature level.
+     * @return              The reason why the feature level is not supported, or Optional.empty if it is supported.
+     */
+    public Optional<String> reasonNotSupported(String featureName, short level) {


The name sort of seems like we're expecting it not to be supported. Maybe call it something like checkIfQuorumSupported? I think it would be good to include "quorum" in here to avoid confusion with the "local" compatibility check

the problem with "check" or "validate" is that it sort of implies that we throw an exception if the validation fails, whereas this returns a reason string.

I liked avoiding exceptions here since it avoids messing with questions like what is the exception type and how do you pull out the string (which is what you really want)

dengziming · 2022-05-27T02:51:35Z

+        FeatureControlManager manager = new FeatureControlManager.Builder().
+                build();
+        manager.updateFeatures(Collections.singletonMap("foo",
+        Map<String, FeatureUpdate.UpgradeType> upgradeTypes,


Is this added by accident?

mumrah · 2022-05-31T15:46:33Z

@cmccabe changes look good. Looks like there is a compile error in FeatureControlManagerTest

cmccabe · 2022-06-01T20:57:30Z

One thing I did just now was get rid of RemoveFeatureLevelRecord since it’s redundant. We can just set level = 0 to remove a feature level

I also fixed it so that we support this case fully. In particular 0 is handled just like any other level — if there are brokers that don’t support level 0, you can’t turn it off. I don't think we'll have any such features any time soon, but this is conceptually simple and will help in the future

mumrah

Removing RemoveFeatureLevelRecord seems fine, I guess we haven't really had any code to produce that record yet so no possibility of deserialization issues.

One minor thing inline, besides that LGTM 👍

mumrah · 2022-06-01T21:00:09Z

        }

-        if (newVersion <= 0) {
+        if (newVersion < 0) {


The error message below should be updated.

cmccabe force-pushed the feature-control-manager-fixes branch from 5d9960f to ed44532 Compare May 25, 2022 17:23

mumrah reviewed May 25, 2022

View reviewed changes

cmccabe added 2 commits May 26, 2022 17:02

address review comments

4be8973

Merge branch 'trunk' into feature-control-manager-fixes

dabac98

dengziming reviewed May 27, 2022

View reviewed changes

cmccabe added 2 commits June 1, 2022 13:40

Merge branch 'trunk' into feature-control-manager-fixes

cba5863

fixes etc

50ae216

mumrah approved these changes Jun 1, 2022

View reviewed changes

cmccabe added 3 commits June 1, 2022 15:00

fix

c40941c

fix2

f70fda6

fix error message

5f3ffad

cmccabe merged commit 0ca9cd4 into apache:trunk Jun 1, 2022

cmccabe deleted the feature-control-manager-fixes branch June 1, 2022 23:09

dengziming mentioned this pull request Jun 2, 2022

KAFKA-13955: Fix failing KRaftClusterTest tests #12238

Merged

3 tasks

dengziming mentioned this pull request Jun 14, 2022

KAFKA-13990: KRaft controller should return right features in ApiVersionResponse #12294

Merged

3 tasks

dengziming mentioned this pull request Aug 24, 2022

KAFKA-13850: Show missing record type in MetadataShell #12103

Merged

3 tasks

Conversation

cmccabe commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe May 26, 2022 • 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!

mumrah commented May 31, 2022

Uh oh!

cmccabe commented Jun 1, 2022

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cmccabe commented May 24, 2022 •

edited

Loading

cmccabe May 26, 2022 •

edited

Loading