You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/05/24 23:46:16 UTC
[GitHub] [kafka] cmccabe opened a new pull request, #12207: MINOR: Several fixes and improvements for FeatureControlManager
cmccabe opened a new pull request, #12207:
URL: https://github.com/apache/kafka/pull/12207
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a diff in pull request #12207: MINOR: Several fixes and improvements for FeatureControlManager
Posted by GitBox <gi...@apache.org>.
dengziming commented on code in PR #12207:
URL: https://github.com/apache/kafka/pull/12207#discussion_r883231041
##########
metadata/src/test/java/org/apache/kafka/controller/FeatureControlManagerTest.java:
##########
@@ -280,7 +261,16 @@ public void testDowngradeMetadataVersion() {
Collections.emptyMap(),
true);
assertEquals(Errors.INVALID_UPDATE_VERSION, result.response().get(MetadataVersion.FEATURE_NAME).error());
- assertEquals("Invalid update version 1 for feature metadata.version. The quorum does not support the given feature version.",
+ assertEquals("Invalid update version 1 for feature metadata.version. Local controller 0 only supports versions 4-5",
result.response().get(MetadataVersion.FEATURE_NAME).message());
}
+
+ @Test
+ public void testSetVersion0ForNonexistentFeature() {
+ FeatureControlManager manager = new FeatureControlManager.Builder().
+ build();
+ manager.updateFeatures(Collections.singletonMap("foo",
+ Map<String, FeatureUpdate.UpgradeType> upgradeTypes,
Review Comment:
Is this added by accident?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on a diff in pull request #12207: MINOR: Several fixes and improvements for FeatureControlManager
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12207:
URL: https://github.com/apache/kafka/pull/12207#discussion_r881925451
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -94,35 +138,6 @@ ControllerResult<Map<String, ApiError>> updateFeatures(
}
}
- ControllerResult<Map<String, ApiError>> initializeMetadataVersion(short initVersion) {
Review Comment:
Yea, this was leftover from a previous iteration on the bootstrapping logic where the controller was just getting the initial metadata version.
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -264,11 +265,11 @@ FinalizedControllerFeatures finalizedFeatures(long epoch) {
}
public void replay(FeatureLevelRecord record) {
- if (!canSupportVersion(record.name(), record.featureLevel())) {
Review Comment:
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
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -234,9 +231,13 @@ private ApiError updateMetadataVersion(
boolean metadataChanged = MetadataVersion.checkIfMetadataChanged(currentVersion, newVersion);
if (!metadataChanged) {
log.info("Downgrading metadata.version from {} to {}.", currentVersion, newVersion);
+ } else if (allowUnsafeDowngrade) {
+ log.info("Downgrading metadata.version unsafely from {} to {}.", currentVersion, newVersion);
Review Comment:
If we want to allow unsafe downgrades we should update the message just below
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -208,19 +218,6 @@ private ApiError updateMetadataVersion(
boolean allowUnsafeDowngrade,
Consumer<ApiMessageAndVersion> recordConsumer
) {
- Optional<VersionRange> quorumSupported = quorumFeatures.quorumSupportedFeature(MetadataVersion.FEATURE_NAME);
- if (!quorumSupported.isPresent()) {
- return invalidMetadataVersion(newVersionLevel, "The quorum does not support metadata.version.");
- }
-
- if (newVersionLevel <= 0) {
Review Comment:
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?
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumFeatures.java:
##########
@@ -75,47 +76,50 @@ public static Map<String, VersionRange> defaultFeatureMap() {
return features;
}
- Optional<VersionRange> quorumSupportedFeature(String featureName) {
- List<VersionRange> supportedVersions = new ArrayList<>(quorumNodeIds.size());
- for (int nodeId : quorumNodeIds) {
- if (nodeId == this.nodeId) {
- // We get this node's features from "supportedFeatures"
- continue;
+ /**
+ * Return the reason a specific feature level is not supported, or Optional.empty if it is supported.
+ *
+ * @param featureName The feature name.
+ * @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) {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on a diff in pull request #12207: MINOR: Several fixes and improvements for FeatureControlManager
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12207:
URL: https://github.com/apache/kafka/pull/12207#discussion_r887295545
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -151,14 +161,14 @@ private ApiError updateFeature(
currentVersion = finalizedVersions.get(featureName);
}
- if (newVersion <= 0) {
+ if (newVersion < 0) {
Review Comment:
The error message below should be updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12207: MINOR: Several fixes and improvements for FeatureControlManager
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12207:
URL: https://github.com/apache/kafka/pull/12207#discussion_r883156221
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -208,19 +218,6 @@ private ApiError updateMetadataVersion(
boolean allowUnsafeDowngrade,
Consumer<ApiMessageAndVersion> recordConsumer
) {
- Optional<VersionRange> quorumSupported = quorumFeatures.quorumSupportedFeature(MetadataVersion.FEATURE_NAME);
- if (!quorumSupported.isPresent()) {
- return invalidMetadataVersion(newVersionLevel, "The quorum does not support metadata.version.");
- }
-
- if (newVersionLevel <= 0) {
Review Comment:
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_3_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.
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -208,19 +218,6 @@ private ApiError updateMetadataVersion(
boolean allowUnsafeDowngrade,
Consumer<ApiMessageAndVersion> recordConsumer
) {
- Optional<VersionRange> quorumSupported = quorumFeatures.quorumSupportedFeature(MetadataVersion.FEATURE_NAME);
- if (!quorumSupported.isPresent()) {
- return invalidMetadataVersion(newVersionLevel, "The quorum does not support metadata.version.");
- }
-
- if (newVersionLevel <= 0) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #12207: MINOR: Several fixes and improvements for FeatureControlManager
Posted by GitBox <gi...@apache.org>.
cmccabe merged PR #12207:
URL: https://github.com/apache/kafka/pull/12207
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on pull request #12207: MINOR: Several fixes and improvements for FeatureControlManager
Posted by GitBox <gi...@apache.org>.
mumrah commented on PR #12207:
URL: https://github.com/apache/kafka/pull/12207#issuecomment-1142306994
@cmccabe changes look good. Looks like there is a compile error in FeatureControlManagerTest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #12207: MINOR: Several fixes and improvements for FeatureControlManager
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12207:
URL: https://github.com/apache/kafka/pull/12207#issuecomment-1144127113
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12207: MINOR: Several fixes and improvements for FeatureControlManager
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12207:
URL: https://github.com/apache/kafka/pull/12207#discussion_r883156749
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumFeatures.java:
##########
@@ -75,47 +76,50 @@ public static Map<String, VersionRange> defaultFeatureMap() {
return features;
}
- Optional<VersionRange> quorumSupportedFeature(String featureName) {
- List<VersionRange> supportedVersions = new ArrayList<>(quorumNodeIds.size());
- for (int nodeId : quorumNodeIds) {
- if (nodeId == this.nodeId) {
- // We get this node's features from "supportedFeatures"
- continue;
+ /**
+ * Return the reason a specific feature level is not supported, or Optional.empty if it is supported.
+ *
+ * @param featureName The feature name.
+ * @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) {
Review Comment:
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12207: MINOR: Several fixes and improvements for FeatureControlManager
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12207:
URL: https://github.com/apache/kafka/pull/12207#discussion_r883174695
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -264,11 +265,11 @@ FinalizedControllerFeatures finalizedFeatures(long epoch) {
}
public void replay(FeatureLevelRecord record) {
- if (!canSupportVersion(record.name(), record.featureLevel())) {
Review Comment:
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.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org