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/19 14:43:22 UTC
[GitHub] [kafka] mumrah opened a new pull request, #12182: MINOR: Use dynamic metadata.version check for KIP-704
mumrah opened a new pull request, #12182:
URL: https://github.com/apache/kafka/pull/12182
Now that `metadata.version` has been integrated with the controller #12050, we need to make use of the dynamic nature of the feature flag. This patch adds a supplier that is passed down to ReplicationControlManager so that the leader recovery feature can be toggled on/off.
IBP/MetadataVersion 3.2 added the `LeaderRecoveryState` tagged field to PartitionRecord. If we are downgrading from 3.2 to 3.0, the downgrade of the feature flag will cause the active controller to stop populating this field, but the value will still be present in existing records. However, since this is a tagged field, it won't cause any compatibility problems when we actually downgrade the server binaries (the tagged field will just be ignored).
--
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 #12182: MINOR: Use dynamic metadata.version check for KIP-704
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12182:
URL: https://github.com/apache/kafka/pull/12182#discussion_r887241653
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -94,21 +94,7 @@ ControllerResult<Map<String, ApiError>> updateFeatures(
}
}
- ControllerResult<Map<String, ApiError>> initializeMetadataVersion(short initVersion) {
Review Comment:
Vestigial code from #12050
--
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
Re: [PR] MINOR: Use dynamic metadata.version check for KIP-704 [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12182: MINOR: Use dynamic metadata.version check for KIP-704
URL: https://github.com/apache/kafka/pull/12182
--
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 #12182: MINOR: Use dynamic metadata.version check for KIP-704
Posted by GitBox <gi...@apache.org>.
mumrah commented on PR #12182:
URL: https://github.com/apache/kafka/pull/12182#issuecomment-1131934558
FeaturesImage has `metadataVersion()` method which we can make accessible through a supplier for the Image/Delta classes. For broker components like ReplicaManager, we can add an argument to applyDelta that indicates a MetadataVersion change. I had some code like this in the [original PR](https://github.com/apache/kafka/pull/11677/commits/83bbf3b21bb0d9f313aa251cf011ec35748876e2#diff-78812e247ffeae6f8c49b1b22506434701b1e1bafe7f92ef8f8708059e292bf0R2110), but we removed it since we didn't have any use cases yet.
--
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
Re: [PR] MINOR: Use dynamic metadata.version check for KIP-704 [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12182:
URL: https://github.com/apache/kafka/pull/12182#issuecomment-2614789731
This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.
--
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
Re: [PR] MINOR: Use dynamic metadata.version check for KIP-704 [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12182:
URL: https://github.com/apache/kafka/pull/12182#issuecomment-2563279410
This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.
<p>
If you are having difficulty finding a reviewer, please reach out on the
[mailing list](https://kafka.apache.org/contact).
<p>
If this PR is no longer valid or desired, please feel free to close it. If no activity
occurs in the next 30 days, it will be automatically closed.
--
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 #12182: MINOR: Use dynamic metadata.version check for KIP-704
Posted by GitBox <gi...@apache.org>.
mumrah commented on PR #12182:
URL: https://github.com/apache/kafka/pull/12182#issuecomment-1137837836
@jsancio I had a Supplier originally, but wanted to have the ability to get the metadata.version at a particular epoch. I think we will need this when we do snapshots before upgrades. Colin's suggestion to just use FeatureControlManager solves that too.
@cmccabe
1) Sounds good, we can mark 3.2-IV0 as didMetadataChange=true. As discussed offline, we won't bump the record version for the records changed on that version since we used tagged fields. That does mean our downgrade logic will need some explicit check on the target version and know to omit those tagged fields.
2) So just include a final `MetadataVersion` in each *Image class? Seems reasonable 👌
3) Yea I think should work for the most part. If we end up exposing the MetadataVersion outside of QuorumController (like into ControllerApis or perhaps into the Raft classes) we may want an abstraction to avoid the FeatureControlManager class leaking out.
I've opened #12214 which I'd like to do prior to this PR. It cleans up some feature flag code
--
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 #12182: MINOR: Use dynamic metadata.version check for KIP-704
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12182:
URL: https://github.com/apache/kafka/pull/12182#issuecomment-1132189652
Thanks for the PR, @mumrah .
1. I agree that tagged fields definitely count as new metadata. `MetadataVersion.IBP_3_2_IV0` is currently marked with `didMetadataChange = false`. If this metadata version did add new metadata (sounds like it did?), this needs to be marked as `true`. Also, we should probably add a very short description of the new fields added in each version, in the comment for the appropriate MetadataVersion. (i.e. "added X field to Y record"). It's good to set this pattern early so others can follow...
2. I would prefer to embed the `MetadataVersion` in each `*Image` (`AclsImage`, `ClusterImage`, etc.) rather than passing it into `write` and other functions. I think it's fundamental to how we interpret the data -- there may be fields that we have to treat slightly differently based on the version (although we want to minimize this as much as we possibly can, of course). This also lets us have invariants in the constructors like AclsImage objects at MV X never have field Y, and so on, which may be nice for unit tests.
3. I think rather than having `MetadataVersionSupplier`, we should just pass `FeatureControlManager` to the constructors of the other Control Managers. This will be useful in the future when we have other feature flags that we want to check. In general I don't see a big benefit to extracting an interface here -- `FeatureControlManager` is very lightweight to create and if you want one for testing, you can just create one. We do this for a lot of other things -- it works very well when you have an object A and B and A makes a lot of calls into B, but B makes none (or very few) into A. This also makes it easier to understand the control flow.
--
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 #12182: MINOR: Use dynamic metadata.version check for KIP-704
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12182:
URL: https://github.com/apache/kafka/pull/12182#discussion_r877220561
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:
##########
@@ -1010,6 +1004,11 @@ private void maybeCompleteAuthorizerInitialLoad() {
}
}
+ // Visible for testing
+ MetadataVersion currentMetadataVersion() {
+ return featureControl.metadataVersion();
+ }
Review Comment:
Oops, that comment was copied over from another PR #12177 :)
--
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] jsancio commented on a diff in pull request #12182: MINOR: Use dynamic metadata.version check for KIP-704
Posted by GitBox <gi...@apache.org>.
jsancio commented on code in PR #12182:
URL: https://github.com/apache/kafka/pull/12182#discussion_r877200466
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:
##########
@@ -1010,6 +1004,11 @@ private void maybeCompleteAuthorizerInitialLoad() {
}
}
+ // Visible for testing
+ MetadataVersion currentMetadataVersion() {
+ return featureControl.metadataVersion();
+ }
Review Comment:
Which test needs this? It doesn't look like this is used in this PR.
--
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 #12182: MINOR: Use dynamic metadata.version check for KIP-704
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12182:
URL: https://github.com/apache/kafka/pull/12182#discussion_r887242125
##########
metadata/src/main/java/org/apache/kafka/controller/ReplicationControlManager.java:
##########
@@ -139,22 +139,17 @@ public class ReplicationControlManager {
static final int MAX_ELECTIONS_PER_IMBALANCE = 1_000;
static class Builder {
- private Supplier<MetadataVersion> metadataVersion = MetadataVersion::latest;
private SnapshotRegistry snapshotRegistry = null;
private LogContext logContext = null;
private short defaultReplicationFactor = (short) 3;
private int defaultNumPartitions = 1;
private int maxElectionsPerImbalance = MAX_ELECTIONS_PER_IMBALANCE;
+ private FeatureControlManager featureControlManager;
Review Comment:
Need FeatureControlManager instead of just a supplier since we need the metadata.version at a particular epoch when creating the snapshot iterator
--
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