kowshik commented on a change in pull request #9001:
URL: https://github.com/apache/kafka/pull/9001#discussion_r498495464
##########
File path:
clients/src/main/java/org/apache/kafka/common/feature/SupportedVersionRange.java
##########
@@ -17,9 +17,16 @@
package org.apache.kafka.common.feature;
import java.util.Map;
+import java.util.Objects;
+import org.apache.kafka.common.utils.Utils;
/**
- * An extended {@link BaseVersionRange} representing the min/max versions for
supported features.
+ * An extended {@link BaseVersionRange} representing the min, max and first
active versions for a
+ * supported feature:
+ * - minVersion: This is the minimum supported version for the feature.
+ * - maxVersion: This the maximum supported version for the feature.
+ * - firstActiveVersion: This is the first active version for the feature.
Versions in the range
Review comment:
@junrao :
I'd like to discuss an example that cites a problem I'm concerned about.
> In general, we tend to deprecate a version very slowly in AK. So, if the
mistake is to deploy a new release that actually deprecates a supported
version. Old clients are likely all gone. So, moving finalized min version to
supported min version may not cause a big problem. We can just document that
people should make sure old versions are no longer used before deploying new
releases.
Let's say we have some feature `F` whose:
* Supported version range is: `[minVersion=1, maxVersion=6]`
* Existing finalized version range in the cluster is: `[minVersionLevel=1,
maxVersionLevel=6]`
Now, let us say a point in time arrives when we need to deprecate the
feature version `1`.
Let us say we bump up supported `minVersion` to `2` in a subsequent major
Kafka release.
Before this new release is deployed, let us assume the cluster operator
knows 100% that old clients that were using the feature at version `1` are
gone, so this is not a problem.
**PROBLEM:** Still, if we deploy this new release, the broker will consider
the following as a feature version incompatibility.
* Supported version range is: `[minVersion=2, maxVersion=6]`
* Existing finalized version range in the cluster is: `[minVersionLevel=1,
maxVersionLevel=6]`
Upon startup of a broker thats using the new release binary, the above
combination will crash the broker since supported `minVersion=2` is greater
than `minVersionLevel=1`. Basically the versioning system thinks that there is
now a broker that does not support `minVersionLevel=1`, which does not adhere
to the rules of the system.
Here is my thought: This is where `firstActiveVersion` becomes useful. By
bumping it up during a release (instead of the supported feature's
`minVersion`), we are able to get past this situation. When
`firstActiveVersion`is advanced in the code, and the cluster is deployed, the
controller (and all brokers) know that the advancement acts a request to the
controller to act upon the feature deprecation (by writing the advanced value
to the `FeatureZNode`). So, in this case we would release the broker with the
supported feature version range: `[minVersion=1, firstActiveVersion=2,
maxVersion=6]`, and the broker release wouldn't fail (because the intent is
clearly expressed to the versioning system).
What are your thoughts on the above?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
