KAFKA-16480: Bump ListOffsets version, IBP version and mark last version of ListOffsets as unstable#15673
KAFKA-16480: Bump ListOffsets version, IBP version and mark last version of ListOffsets as unstable#15673clolov wants to merge 10 commits intoapache:trunkfrom
Conversation
|
Yup, I need to change the kafka-get-offsets tool to easily access said functionality, but I am in the process of raising that PR |
84a6049 to
35a1cb0
Compare
|
Hello @junrao! Yes, I had missed updating some of the tests - I believe the majority of them should now be fixed, but I will circle back once the latest build passes to check again |
junrao
left a comment
There was a problem hiding this comment.
@clolov : Thanks for the updated PR. A couple of more comments.
Also, is 3.8-IV0 referenced in
MetadataVersionTest
and 3_8_IV0 is referenced in
QuorumControllerTest
PartitionChangeBuilderTest
PartitionRegistrationTest
BrokerMetadataPublisherTest
ZkMigrationIntegrationTest
Should we change all those references to 3.8-IV1?
| else if (requireTimestamp) | ||
| minVersion = 1; | ||
| return new Builder(minVersion, ApiKeys.LIST_OFFSETS.latestVersion(), CONSUMER_REPLICA_ID, isolationLevel); | ||
| return new Builder(minVersion, ApiKeys.LIST_OFFSETS.latestVersion(false), CONSUMER_REPLICA_ID, isolationLevel); |
There was a problem hiding this comment.
Hmm, does that mean we can't test the latest unstable version in the client?
There was a problem hiding this comment.
We cannot test the latest unstable version in the client, correct. This should be only temporary until I release the subsequent pull request where we introduce a correct OffsetSpec to allow clients to correctly call this behaviour and introduce tests for it
| TestUtils.produceMessage(servers, topic, "test-10", System.currentTimeMillis() + 10L) | ||
|
|
||
| for (version <- ApiKeys.LIST_OFFSETS.oldestVersion to ApiKeys.LIST_OFFSETS.latestVersion) { | ||
| for (version <- ApiKeys.LIST_OFFSETS.oldestVersion to ApiKeys.LIST_OFFSETS.latestVersion(false)) { |
There was a problem hiding this comment.
Do we have tests that enable the latest unstable version?
There was a problem hiding this comment.
I want to hide the latest unstable version of the ListOffsetRequest from everywhere. A follow-up pull request will allow it to be called and introduce the changes needed for the client to call with this a newer OffsetSpec and a test confirming the behaviour. Does this make sense or am I misunderstanding the question?
|
@clolov: Are you able to address the remaining comments? 3.8.0 code freeze is getting close. Thanks. |
|
Heya, apologies, I will get to addressing these this weekend! |
35a1cb0 to
4a3600f
Compare
|
Heya @junrao! Let me know your thoughts on my responses. I have futher updated the below + rebased. I am waiting on the tests to finish running and if they uncover something I will aim to remedy it today. MetadataVersionTest - I added 3.8-IV1 where I believe it is needed |
| MetadataVersion.IBP_3_7_IV1, | ||
| MetadataVersion.IBP_3_7_IV2, | ||
| MetadataVersion.IBP_3_8_IV0 | ||
| MetadataVersion.IBP_3_8_IV1 |
There was a problem hiding this comment.
Should we make the same change in PartitionChangeBuilderTest?
There was a problem hiding this comment.
My miss, updated in the next commit
There was a problem hiding this comment.
This corresponds to ELR and needs to be changed to BP_4_0_IV0 like PartitionChangeBuilderTest.java. Ditto below.
@CalvinConfluent : Could you confirm that? Also, could you check if there is any other change needed for ELR in this PR?
There was a problem hiding this comment.
QuorumControllerTest.java: testUncleanShutdownBroker
| setClusterId(active.clusterId()). | ||
| setIncarnationId(Uuid.fromString("kxAT73dKQsitIedpiPtwBA")). | ||
| setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_3_8_IV0)). | ||
| setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_3_8_IV1)). |
There was a problem hiding this comment.
Should we change IBP_3_8_IV0 to IBP_3_8_IV1 in testUncleanShutdownBroker()? It is testing ELR, but we want to make sure ELR works under the latest version too, right?
There was a problem hiding this comment.
Hopefully this ought to be addressed in the next commit as well
4a3600f to
09bad16
Compare
|
I will look at |
09bad16 to
53276b1
Compare
|
Heya @junrao after inspecting some of the tests I think I have misunderstood how marking an API version as unstable works. From a discussion with Nikhil I think we can only do one of two things - we can either keep the IBP and mark the new version as unstable or we can bump the IBP. Otherwise what happens is that the broker treats the version as non-existent while clients don't respect the configuration and still send the new version. Is this your understanding as well? If it is, then I believe I will just mark the version as unstable while I make the client changes and then I will mark it as stable and bump the IBP all in one commit. |
|
Given that we are marking the latest version as unstable in ListOffsetsResponse, it would make sense to not bump the IBP now as @clolov suggests, and only bump it when ListOffsetsResponse v9 is ready to be marked as stable. Doing this allows us to test v9 with |
|
@clolov @nikramakrishnan : If an RPC is only used by the client, we don't need to bump up the IBP. However, ListOffsetRequest is used by both the client and the broker. If we don't bump the IBP, we can't test the logic for the new ListOffsetRequest on the broker, right?
Hmm, normally, a client first sends an ApiVersionRequest to the broker to get exposed API versions. The broker decides whether to expose the latest version based on |
|
@clolov : 3.8.0 feature freeze is about 1 week away. It would be useful to get this PR in before the 3.8 branch is cut. Any updates on this PR? What's an example of the failed test? |
|
@clolov Sorry to ping you again. The 3.8 branch is going to be cut by Friday. Will you still be able to complete this PR for 3.8.0? |
|
Heya @junrao, the delay is entirely my bad, I will aim to give an update either today or early tomorrow morning |
53276b1 to
c44e792
Compare
|
I have scrapped the unstable flag. I will raise the final changes needed for the CLI command over the weekend - happy for those to be applied on top of the newly cut 3.8 branch. |
| * IT CANNOT BE CHANGED.</strong> | ||
| */ | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_3_7_IV4; | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_3_8_IV1; |
There was a problem hiding this comment.
@CalvinConfluent and @cmccabe : Is IBP_3_8_IV0 for KIP-966 production ready?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks, @CalvinConfluent .
@clolov : In that case, we will need to (1) mark IBP_3_8_IV0 as unused, (2) create a new MV IBP_4_0_IV0, (3) replace existing references of IBP_3_8_IV0 related to Elr like the following to IBP_4_0_IV0.
public boolean isElrSupported() {
return this.isAtLeast(IBP_3_8_IV0);
}
There was a problem hiding this comment.
Okay, I believe I have moved ELR to at least IBP_4_0_IV0, but I would prefer to leave the rest of the test changes for when ELR is launched i.e. I have only changed the tests to reflect the changes needed for 3.8.1
There was a problem hiding this comment.
The 4.0 IV0 is taken, I think we should use IV1?
// Introduce version 1 of the GroupVersion feature (KIP-848).
IBP_4_0_IVO(21, "4.0", "IV0", false);
There was a problem hiding this comment.
@jolshan: What's the benefit of reusing 3.8-IV0 for ListOffset instead of creating a new 3.8-IV1? To me, creating 3.8-IV1 makes it a bit easier to understand the change history. It leaves an IV unused, but that doesn't seem a big concern.
There was a problem hiding this comment.
I'm not sure I understand the benefit of leaving it unused. I would suspect if there are many ongoing features, we will have a lot of unused versions and that is confusing it its own way. I was hoping the details of KIP-1014 would be finalized by now so that there was clear guidance for this sort of state.
There was a problem hiding this comment.
ELR was originally associated with IBP_3_7_IV3. When it's moved IBP_3_8_IV0, we left IBP_3_7_IV3 unused since IBP_3_7_IV4 has already been implemented. We didn't reuse IBP_3_7_IV3. Here, there is no 3.8 IV after IBP_3_8_IV0. So, we could reuse IBP_3_8_IV0 for ListOffset. However, it seems that a simpler and more consistent policy is to never reuse an IV?
There was a problem hiding this comment.
Heh, this comment thread is becoming the KIP-1014 discussion. I suppose while we have not completed that discussion we can just pick an approach for now. I don't feel super strongly about this for the one more MV while we don't have the conclusive answer.
There was a problem hiding this comment.
There is really no reason to leave unstable MVs unused. I think the issue with IBP_3_7_IV3 was a misunderstanding. The whole point of an unstable MV is you can do whatever you want (until it becomes stable).
|
|
||
| val debugReplicaRequest = ListOffsetsRequest.Builder | ||
| .forReplica(ApiKeys.LIST_OFFSETS.latestVersion, ListOffsetsRequest.DEBUGGING_REPLICA_ID) | ||
| .forReplica(ApiKeys.LIST_OFFSETS.latestVersion(), ListOffsetsRequest.DEBUGGING_REPLICA_ID) |
There was a problem hiding this comment.
Why do we need to add the brackets?
There was a problem hiding this comment.
We do not, I will raise another version tomorrow morning which address this + fix the merge conflicts
22fb85d to
f3ab48a
Compare
| // Version 8 enables listing offsets by local log start offset (KIP-405). | ||
| "validVersions": "0-8", | ||
| // | ||
| // Version 9 enables listing offsets by last tiered offset (KIP-1005). |
There was a problem hiding this comment.
To let the AdminClient use this, we need to add a new type of OffsetSpec and a way to set oldestAllowedVersion in ListOffsetsRequest.Build to version 9, right? See https://github.com/apache/kafka/pull/10760/files
There was a problem hiding this comment.
Yes, correct, but I want to get this PR in before I move to that. I do not want to bunch all of these changes in the same PR
| assertEquals(IBP_3_7_IV4, MetadataVersion.fromVersionString("3.7-IV4")); | ||
|
|
||
| assertEquals(IBP_3_8_IV0, MetadataVersion.fromVersionString("3.8-IV0")); | ||
| assertEquals(IBP_3_8_IV1, MetadataVersion.fromVersionString("3.8-IV1")); |
There was a problem hiding this comment.
Should we add a comment that IBP_3_8_IV1 is the latest product version of 3.8?
| assertEquals("3.7-IV3", IBP_3_7_IV3.version()); | ||
| assertEquals("3.7-IV4", IBP_3_7_IV4.version()); | ||
| assertEquals("3.8-IV0", IBP_3_8_IV0.version()); | ||
| assertEquals("3.8-IV1", IBP_3_8_IV1.version()); |
There was a problem hiding this comment.
- Should we add
IBP_3_8_IV1intestShortVersion()too? - Should we change
testIsElrSupported()to useIBP_4_0_IVO? - Should we update
FeatureCommandTest.testDescribeWithKRaftAndBootstrapControllersfrommetadataVersion = MetadataVersion.IBP_3_7_IV4tometadataVersion = MetadataVersion.IBP_3_8_IV1?
| TEST_1(1, MetadataVersion.IBP_3_7_IV0, Collections.emptyMap()), | ||
| // TEST_2 released right before MV 3.8-IVO was released, and it depends on this metadata version | ||
| TEST_2(2, MetadataVersion.IBP_3_8_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_3_8_IV0.featureLevel())); | ||
| TEST_2(2, MetadataVersion.IBP_4_0_IVO, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_4_0_IVO.featureLevel())); |
There was a problem hiding this comment.
@jolshan : Should we keep the MV for TEST_2 fixed at IBP_3_8_IV0?
There was a problem hiding this comment.
Yeah, I was thinking about this. If we should always have this version be an unstable version.
I think there are trade offs either way. We get the unstable version test coverage, but we have to keep updating it every time the MV becomes stable.
There was a problem hiding this comment.
I have a plan for this. I plan to keep the latest version unstable and create a TEST_3. TEST_2 will be used for the dependency test and it can be either 3.8 or 4.0.
| return MetadataVersion.IBP_3_7_IV2; | ||
| case (short) 2: | ||
| return MetadataVersion.IBP_3_8_IV0; | ||
| return MetadataVersion.IBP_3_8_IV1; |
There was a problem hiding this comment.
We need to change it to return IBP_4_0_IVO for version 2 of PartitionChangeRecord since the version 2 change is for ELR.
f3ab48a to
5dabbe6
Compare
| // Change expected message to reflect latest MetadataVersion (SupportedMaxVersion increases when adding a new version) | ||
| assertEquals("Feature: metadata.version\tSupportedMinVersion: 3.0-IV1\t" + | ||
| "SupportedMaxVersion: 4.0-IV0\tFinalizedVersionLevel: 3.3-IV1\t", outputWithoutEpoch(features.get(1))); | ||
| "SupportedMaxVersion: 3.8-IV1\tFinalizedVersionLevel: 3.3-IV1\t", outputWithoutEpoch(features.get(1))); |
There was a problem hiding this comment.
I haven't had the time to fully check why this had to change from 4.0-IV0 to 3.8-IV1, but I will circle back tomorrow to confirm should you think it is a problem
There was a problem hiding this comment.
The test seems to fail with this change. This test is configured with unstable.feature.versions.enable=true. So, returning 4.0-IV0 for max supported version is correct.
There was a problem hiding this comment.
Yeah, upon rebasing this does need to be 4.0-IV0, I suspect my workspace was confused when I ran the test locally. Reverted!
|
|
||
| // Introduce version 1 of the GroupVersion feature (KIP-848). | ||
| IBP_4_0_IV0(21, "4.0", "IV0", false); | ||
| IBP_4_0_IV0(22, "4.0", "IV0", false); |
There was a problem hiding this comment.
Perhaps we could add a comment that IBP_4_0_IV0 also corresponds to ELR for now. @CalvinConfluent : Is this ok with you for now?
| // Change expected message to reflect latest MetadataVersion (SupportedMaxVersion increases when adding a new version) | ||
| assertEquals("Feature: metadata.version\tSupportedMinVersion: 3.0-IV1\t" + | ||
| "SupportedMaxVersion: 4.0-IV0\tFinalizedVersionLevel: 3.3-IV1\t", outputWithoutEpoch(features.get(1))); | ||
| "SupportedMaxVersion: 3.8-IV1\tFinalizedVersionLevel: 3.3-IV1\t", outputWithoutEpoch(features.get(1))); |
There was a problem hiding this comment.
The test seems to fail with this change. This test is configured with unstable.feature.versions.enable=true. So, returning 4.0-IV0 for max supported version is correct.
| MetadataVersion.IBP_3_7_IV1, | ||
| MetadataVersion.IBP_3_7_IV2, | ||
| MetadataVersion.IBP_3_8_IV0 | ||
| MetadataVersion.IBP_3_8_IV1 |
There was a problem hiding this comment.
This corresponds to ELR and needs to be changed to BP_4_0_IV0 like PartitionChangeBuilderTest.java. Ditto below.
@CalvinConfluent : Could you confirm that? Also, could you check if there is any other change needed for ELR in this PR?
| TEST_1(1, MetadataVersion.IBP_3_7_IV0, Collections.emptyMap()), | ||
| // TEST_2 released right before MV 3.8-IVO was released, and it depends on this metadata version | ||
| TEST_2(2, MetadataVersion.IBP_3_8_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_3_8_IV0.featureLevel())); | ||
| TEST_2(2, MetadataVersion.IBP_4_0_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_4_0_IV0.featureLevel())); |
There was a problem hiding this comment.
Could we adjust the comment above on "MV 3.8-IVO"?
5dabbe6 to
778949b
Compare
| setBrokerId(brokerId). | ||
| setClusterId(active.clusterId()). | ||
| setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_3_8_IV0)). | ||
| setFeatures(brokerFeatures(MetadataVersion.IBP_3_0_IV1, MetadataVersion.IBP_3_8_IV1)). |
There was a problem hiding this comment.
Hmm, this is testing ELR. So it seems that it needs to be IBP_4_0_IV0. Ditto below. @CalvinConfluent : Could you confirm this?
There was a problem hiding this comment.
Yes, it requires IBP_4_0_IV0
| List<UnwritableMetadataException> exceptions = new ArrayList<>(); | ||
| ImageWriterOptions options = new ImageWriterOptions.Builder(). | ||
| setMetadataVersion(MetadataVersion.IBP_3_8_IV0). | ||
| setMetadataVersion(MetadataVersion.IBP_3_8_IV1). |
There was a problem hiding this comment.
Hmm, this is testing ELR. So it seems that it needs to be IBP_4_0_IV0. @CalvinConfluent : Could you confirm this?
There was a problem hiding this comment.
Yes, it requires IBP_4_0_IV0
| TEST_1(1, MetadataVersion.IBP_3_7_IV0, Collections.emptyMap()), | ||
| // TEST_2 released right before MV 3.8-IVO was released, and it depends on this metadata version | ||
| TEST_2(2, MetadataVersion.IBP_3_8_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_3_8_IV0.featureLevel())); | ||
| // TEST_2 released right before MV 4.0-IVO was released, and it depends on this metadata version |
There was a problem hiding this comment.
4.0-IVO => 4.0-IV0 This is an existing issue. The trailing number should be 0, not an O.
Ditto for 3.7-IVO above.
| * IT CANNOT BE CHANGED.</strong> | ||
| */ | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_3_7_IV4; | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_3_8_IV1; |
There was a problem hiding this comment.
@jolshan : Just to clarify. Are you saying that ELR can just use MV 4.0 IV0, instead of creating a new one?
…fsets as unstable
778949b to
8b6df23
Compare
| // TEST_2 released right before MV 3.8-IVO was released, and it depends on this metadata version | ||
| TEST_2(2, MetadataVersion.IBP_3_8_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_3_8_IV0.featureLevel())); | ||
| // TEST_2 released right before MV 4.0-IV0 was released, and it depends on this metadata version | ||
| TEST_2(2, MetadataVersion.IBP_4_0_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_4_0_IV0.featureLevel())); |
There was a problem hiding this comment.
@jolshan left a comment earlier. #15673 (comment)
Since she plans to create a new feature TEST_3 for unstable version. We probably can keep this at 3_8_IV1 since it's a stable version.
There was a problem hiding this comment.
Ah, apologies, I missed Justine's comment, I will make the change now, thank you 😊
There was a problem hiding this comment.
Thanks, Christo. Have you pushed the change?
There was a problem hiding this comment.
I put this to 3.8-IV1, but since 3.8-IV1 is now marked as a production version a test (testLatestFeaturesWithOldMetadataVersion) fails with a message like
test.feature.version could not be set to 2 because it depends on metadata.version level 21
And two tests (testUnstableTestVersion, testUnstableFeatureThrowsError) fail with
Expected java.lang.IllegalArgumentException to be thrown, but nothing was thrown.
As such, I have a preference that we keep this as IBP_4_0_IV0 which appears to exercise all scenarios successfully.
What is your opinion @jolshan? Do you want a stable version to be exercised as part of TEST_2 right now?
| * IT CANNOT BE CHANGED.</strong> | ||
| */ | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_3_7_IV4; | ||
| public static final MetadataVersion LATEST_PRODUCTION = IBP_3_8_IV1; |
There was a problem hiding this comment.
@jolshan: What's the benefit of reusing 3.8-IV0 for ListOffset instead of creating a new 3.8-IV1? To me, creating 3.8-IV1 makes it a bit easier to understand the change history. It leaves an IV unused, but that doesn't seem a big concern.
|
The latest test failures, in my opinion, are unrelated to the change |
|
3.8 is done at this point. These changes can be done in 3.9-IV0 once #16347 is in. |
cmccabe
left a comment
There was a problem hiding this comment.
I'm leaving a placeholder -1 here until we can understand this better.
-
Does this add a new feature to 3.8 or not? Some comments seems to indicate that it does, but there was also a comment that this was just "fixing a mistake"
-
Is the last version of ListOffsets unstable or not? Title of the PR indicates that it should be, but the PR itself doesn't set
"latestVersionUnstable": true.in ListOffsetsRequest.
thanks, all
|
@cmccabe and @jlprat : Here is a summary of of the issue that this PR is trying to fix. KIP-1005 proposes to (1) add support for latest-tiered timestamp (-5) in ListOffset request (2) expose earliest-local (-4) and latest-tiered timestamp (-5) in AdminClient and GetOffsetShell tool. (1) was implemented in the 3.8 branch, but was implemented incorrectly since it didn't bump up the request version (see https://issues.apache.org/jira/browse/KAFKA-16480). (2) hasn't been implemented yet.
For this PR, we just want to fix (1).
The latest PR makes the latest version ListOffsets stable. So, we need to change the description of this PR. |
|
I commented in the other PR that I think we should just revert KIP-1005 from 3.8 if it's not finished. #16347 (comment) We can implement it in 3.9-IV0. What do you think? |
|
Sure, I am okay with reverting the ListOffsetRequest. If #16347 isn't merge by tomorrow morning my time I will provide my review! |
Summary
This is a follow-up of #15213 where I missed updating the API version and the IBP version. I chose to mark the latest version of the ListOffsets API as unstable until the whole KIP-1005 is implemented