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 Thinking about it again, I see a way forward here. The key thing 
seems to be that you feel it is rare to deprecate feature versions in AK. I 
agree with the same. So, I propose we just do not have to solve that 
deprecation problem in this PR, until we find a clear route that the AK 
community agrees with.
   
   In this PR I propose that I revert the `firstActiveVersion` change, leaving 
the rest of the things the way they are. In the future, we can develop a 
concrete solution for version deprecation i.e. advancing `minVersion` of 
supported feature, potentially using `firstActiveVersion` or other ways (it is 
up for discussion, maybe in a separate KIP). What do you feel?
   
   Going back to your point:
   > 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.
   
   There is a consequence to relaxing the current check:
   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 the 
`minVersionLevel` at a particular value. It seems useful to keep the concept of 
`minVersionLevel` like the way it is now (i.e. it is the lowest version 
guaranteed to be supported by any broker in the cluster for a feature). And as 
I said above, in the future, we can decide on ways to mutate it safely (maybe 
through `firstActiveVersion` or other means).
   
   




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


Reply via email to