kowshik commented on a change in pull request #9001:
URL: https://github.com/apache/kafka/pull/9001#discussion_r498574911
##########
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 My response below.
> I was thinking what if we relax the current check by just making sure that
maxVersion of finalized is within the supported range. Basically in your
example, if supported minVersion goes to 2, it's still allowed since it's less
than maxVersion of finalized. However, if supported minVersion goes to 7, this
fails the broker since it's more than maxVersion of finalized.
If we relaxed the current check like you suggested, then, as of today the
following will happen:
The controller can not effectively finalize `minVersionLevel` for the
feature, because, with a relaxed check we do not know whether all brokers in
the cluster support a particular `minVersion` when the controller finalizes it
at a particular value.
Supposing we want to keep the concept of `minVersionLevel` like the way it
is now (i.e. it tells you the lowest feature version level guaranteed to be
supported by any broker in the cluster), then, `firstActiveVersion` provides
the door to mutate it safely whenever the need arises.
So when thinking about your suggestion, it is not clear to me if you are
intending to alter the meaning of `minVersionLevel`? If so, please could you
tell me what is the new meaning of `minVersionLevel` that you are proposing?
> Your concern for the relaxed check seems to be around deploying a wrong
version of the broker by mistake.
Not exactly, my concern is a bit different. It is to be noted that
`minVersionLevel` can not be changed through the `UPDATE_FEATURES` api.
Assuming we want to maintain the meaning of `minVersionLevel` the way it is
today, my concern is about how would one alter `minVersionLevel` for a feature
when the need arises? This needs to be done without letting the versioning
system misconstrue the intent with a feature version incompatibility. This is
where usage of `firstActiveVersion` acts as a solution.
----------------------------------------------------------------
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:
[email protected]
