KAFKA-10729: Bump remaining RPC's to use tagged fields. by gardnervickers · Pull Request #9601 · apache/kafka

gardnervickers · 2020-11-16T19:52:21Z

As a follow-up from KIP-482, this PR bumps the version for several
RPC's to enable tagged fields via the flexible versioning mechanism.

Additionally, a new IBP version KAFKA_2_8_IV0 is introduced to
allow replication to take advantage of these new RPC versions for
OffsetForLeaderEpoch and ListOffset.

ijuma · 2020-11-16T21:53:37Z

2.7 has been branched. It should be 2.8, right?

Thanks, that makes more sense.

As a follow-up from KIP-482, this PR bumps the version for several RPC's to enable tagged fields via the flexible versioning mechanism. Additionally, a new IBP version `KAFKA_2_7_IV3` is introduced to allow replication to take advantage of these new RPC versions for OffsetForLeaderEpoch and ListOffset.

gardnervickers · 2020-11-18T18:58:22Z

OffsetForLeaderEpoch and Produce are not yet generated RPCs, but will be once #9401 and #9547 are merged. I've removed the taggedFields bump for these RPC's. We can bump them once their respective PR's are merged.

OffsetForLeaderEpoch and Produce are not yet generated RPCs, but will be once apache#9401 and apache#9547 are merged.

dajac · 2020-11-19T10:45:09Z

@gardnervickers #9401 and #9547 have been merged. You can bring them back in this PR if you like.

…-versions

This reverts commit b836a3d.

hachikuji · 2020-11-19T16:57:47Z

    // Introduced AlterIsr (KIP-497)
-    KAFKA_2_7_IV2
+    KAFKA_2_7_IV2,
+    // Flexible versioning on ListOffsets


For what it's worth, WriteTxnMarkers and OffsetsForLeaderEpoch are also inter-broker APIs.

…aderEpoch. - Allow using RecordBatch v2 for ProduceRequestV9.

gardnervickers · 2020-11-29T18:45:02Z

This is to fix testThrottlingNotEnabledForConnectionToOlderBroker. The test forces ApiVersionsResponse to version 5, but relied on the fact that nothing really changed between version 5 and version 8 for PRODUCE responses. With flex versions we need to ensure the response matches the ApiVersions response

gardnervickers · 2020-11-29T18:46:57Z

Having this override seemed a bit error prone, and was causing failures for the NetworkClientTest. I opted to remove it entirely in favor of forcing the caller to specify the response version.

gardnervickers · 2020-11-29T18:50:08Z

Admittedly I'm not sure why we check the version of the last OffsetForLeaderEpoch response is 3 here. I decided to widen the check a bit so this won't break for future versions.

I think this was originally using 1 in order to ensure that we were using a version which included the epoch in the response. Since then it looks like it has been updated blindly every time we've bumped the protocol. I'm ok leaving this as is, but we could probably also get rid of it.

- NetworkClientTest.testThrottlingNotEnabledForConnectionToOlderBroker was relying on the latest PRODUCE version being unchanged from version 5. Fix this by supplying the version when constructing the throttled produce response. - Fixed AlterReplicaLogDirsResponse to take the version in the constructor instead of offering an override which assumes that the most recent version is in use. This fixes the NetworkClientTest. - Fixed ReplicaFetcherThreadTest.shouldFetchLeaderEpochSecondTimeIfLeaderRepliesWithEpochNotKnownToFollower to check that the lastUsedOffsetForLeaderEpochVersion is >= 3 instead of == 3. It seems this check is mostly in place to ensure that a OffsetForLeaderEpoch response was sent.

…-versions

hachikuji · 2020-12-01T02:30:34Z

            case 6:
            case 7:
            case 8:
+            case 9:


I wonder if we may as well make this the default case. Not sure we're getting much by forcing ourselves to update this logic after each bump. Maybe the range validation is still useful, but that could be done by using oldestVersion and latestVersion.

hachikuji · 2020-12-01T02:35:04Z

-        public Builder(final List<TxnMarkerEntry> markers) {
-            super(ApiKeys.WRITE_TXN_MARKERS);
+        public Builder(final List<TxnMarkerEntry> markers, short latestAllowedVersion) {
+            super(ApiKeys.WRITE_TXN_MARKERS, ApiKeys.WRITE_TXN_MARKERS.oldestVersion(), latestAllowedVersion);


I think this is probably ok, but it is a little inconsistent with how we handle the versions for other inter-broker RPCs. Since we rely on the IBP, we always set the version explicitly in the caller, which means there is exactly one allowable version for the builder to use. See for example LeaderAndIsrRequest.Builder.

hachikuji · 2020-12-01T02:37:05Z

        4 /* correlation id */ +
-        Type.NULLABLE_STRING.sizeOf(clientId) /* client id */
+        Type.NULLABLE_STRING.sizeOf(clientId)  /* client id */ +
+        (if (flexVersion) ByteUtils.sizeOfUnsignedVarint(0) else 0)


nit: maybe add a comment that this field is for the number of tagged fields?

- Make the most recent record batch version the default case when selecting a record batch version. Check that the version is within the existing version bounds. - Force a specific version in the WriteTxnMarkersRequest constructor to match other requests which utilize the IBP like LeaderAndISR.

…sion in the RequestResponseTest

hachikuji

LGTM. Thanks for the patch!

…t-for-generated-requests * apache-github/trunk: KAFKA-9263 The new hw is added to incorrect log when ReplicaAlterLogDirsThread is replacing log (fix PlaintextAdminIntegrationTest.testAlterReplicaLogDirs) (apache#9423) KAFKA-10729; Bump remaining RPC's to use tagged fields. (apache#9601) clients/src/main/java/org/apache/kafka/common/requests/AbstractResponse.java clients/src/main/java/org/apache/kafka/common/requests/AlterReplicaLogDirsResponse.java clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java

ijuma reviewed Nov 16, 2020

View reviewed changes

gardnervickers force-pushed the list-offset-flex-versions branch from 10bc65b to 1488283 Compare November 17, 2020 01:06

gardnervickers force-pushed the list-offset-flex-versions branch from 1488283 to fb11509 Compare November 17, 2020 01:09

gardnervickers changed the title ~~MINOR: Enable flexible versioning for ListOffsetRequest/ListOffsetResponse.~~ KAFKA-10729: Bump remaining RPC's to use tagged fields. Nov 17, 2020

gardnervickers force-pushed the list-offset-flex-versions branch from fe69332 to 31f53b4 Compare November 18, 2020 18:58

Revert using flexibleVersions for non-generated RPC's

b836a3d

OffsetForLeaderEpoch and Produce are not yet generated RPCs, but will be once apache#9401 and apache#9547 are merged.

gardnervickers force-pushed the list-offset-flex-versions branch from 31f53b4 to b836a3d Compare November 18, 2020 18:59

gardnervickers added 2 commits November 19, 2020 08:39

Merge branch 'trunk' of github.com:apache/kafka into list-offset-flex…

27c5b77

…-versions

Revert "Revert using flexibleVersions for non-generated RPC's"

38f2747

This reverts commit b836a3d.

hachikuji reviewed Nov 19, 2020

View reviewed changes

gardnervickers added 2 commits November 20, 2020 14:06

- Use KAFKA_2_8_IV0 for ListOffsets, WriteTxnMarkers and OffsetsForLe…

0dfa059

…aderEpoch. - Allow using RecordBatch v2 for ProduceRequestV9.

Fix the EdgeCaseRequestTest to take into account flexible versions.

ccc6345

jolshan mentioned this pull request Nov 22, 2020

KAFKA-10547; add topicId in MetadataResp #9622

Merged

3 tasks

gardnervickers force-pushed the list-offset-flex-versions branch from 59b6726 to bb5f2d5 Compare November 29, 2020 18:40

gardnervickers commented Nov 29, 2020

View reviewed changes

gardnervickers force-pushed the list-offset-flex-versions branch from bb5f2d5 to 7a7d5fd Compare November 29, 2020 21:27

Merge branch 'trunk' of github.com:apache/kafka into list-offset-flex…

6f0ed04

…-versions

hachikuji reviewed Dec 1, 2020

View reviewed changes

gardnervickers added 2 commits December 1, 2020 12:42

Use LIST_OFFSET.lastestVersion() instead of hardcoding the latest ver…

fb9fbe8

…sion in the RequestResponseTest

hachikuji approved these changes Dec 1, 2020

View reviewed changes

hachikuji merged commit 85f94d5 into apache:trunk Dec 1, 2020

Conversation

gardnervickers commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gardnervickers commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dajac commented Nov 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gardnervickers commented Nov 16, 2020 •

edited

Loading

gardnervickers commented Nov 18, 2020 •

edited

Loading

hachikuji Dec 1, 2020 •

edited

Loading