KAFKA-10818: Skip conversion to `Struct` when serializing generated requests/responses by ijuma · Pull Request #7409 · apache/kafka

ijuma · 2019-09-29T03:30:10Z

Generated request/response classes have code to serialize/deserialize directly to
ByteBuffer so the intermediate conversion to Struct can be skipped for them.
We have recently completed the transition to generated request/response classes,
so we can also remove the Struct based fallbacks.

Additional noteworthy changes:

AbstractRequest.parseRequest has a more efficient computation of request size that
relies on the received buffer instead of the parsed Struct.
Use SendBuilder for AbstractRequest/Response toSend, made the superclass
implementation final and removed the overrides that are no longer necessary.
Removed request/response constructors that assume latest version as they are unsafe
outside of tests.
Removed redundant version fields in requests/responses.
Removed unnecessary work in OffsetFetchResponse's constructor when version >= 2.
Made AbstractResponse.throttleTimeMs() abstract.
Using toSend in SaslClientAuthenticator instead of serialize.
Various changes in Request/Response classes to make them more consistent and to
rely on the Data classes as much as possible when it comes to their state.
Remove the version argument from AbstractResponse.toString.
Fix getErrorResponse for ProduceRequest and DescribeClientQuotasRequest to
use ApiError which processes the error message sent back to the clients. This was
uncovered by an accidental fix to a RequestResponseTest test (it was calling
AbstractResponse.toString instead of AbstractResponse.toString(short)).

Rely on existing protocol tests to ensure this refactoring does not change
observed behavior (aside from improved performance).

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ijuma · 2019-09-29T03:45:30Z

Started a system tests run: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/3052/

ijuma · 2019-09-30T03:21:08Z

retest this please

ijuma · 2019-09-30T12:24:02Z

Scala 2.13 build passed, Scala 2.12 timed out, 2.11 failed due to H40 slave being offline.

ijuma · 2019-09-30T12:24:08Z

retest this please

ijuma · 2019-10-02T05:15:17Z

Need to verify that this condition is equivalent to the previous behavior of checking if the field exists.

ijuma · 2019-10-02T12:18:17Z

retest this please

ijuma · 2019-10-03T22:19:00Z

retest this please

… on the auto generated protocol

…o generated protocol

…ions

…ator`.

…ead of structs

…uses the auto generated protocol

…r clarity

…ToByteBuffer

…t-for-generated-requests * apache-github/trunk: MINOR: Fix flaky test shouldQueryOnlyActivePartitionStoresByDefault (apache#9681) KAFKA-10799 AlterIsr utilizes ReplicaManager ISR metrics (apache#9677) MINOR: Fix KTable-KTable foreign-key join example (apache#9683) KAFKA-10473: Add docs on partition size-on-disk, and other log-related metrics (apache#9276) KAFKA-10739; Replace EpochEndOffset with automated protocol (apache#9630) KAFKA-10460: ReplicaListValidator format checking is incomplete (apache#9326) KAFKA-10554; Perform follower truncation based on diverging epochs in Fetch response (apache#9382) MINOR: Align the UID inside/outside container (apache#9652) KAFKA-10794 Replica leader election is too slow in the case of too many partitions (apache#9675) KAFKA-10090 Misleading warnings: The configuration was supplied but i… (apache#8826) clients/src/main/java/org/apache/kafka/common/requests/OffsetsForLeaderEpochResponse.java clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetcherTest.java core/src/test/scala/unit/kafka/server/epoch/util/ReplicaFetcherMockBlockingSend.scala

Also consolidate and clean-up.

ijuma · 2020-12-04T06:55:21Z

+        AbstractResponse response = AbstractResponse.parseResponse(apiKey, buffer, apiVersion);
+
+        // We correlate after parsing the response to avoid spurious correlation errors when receiving malformed
+        // responses


TODO: Handle this better.

ijuma · 2020-12-04T14:25:01Z

Looks like the failures are unrelated, so this is ready for review @chia7712. The PR description needs a few tweaks, I will do that tomorrow.

ijuma · 2020-12-04T14:43:13Z

Actually, I went ahead and updated the PR description.

chia7712

@ijuma Thanks for this huge patch. Some minor comments are left. Please take a look.

chia7712 · 2020-12-04T14:40:15Z

+    }
+
+    // Visible for testing
+    public final ByteBuffer serializeWithHeader(RequestHeader header) {


How about moving this method to test code? MessageTestUtil could be a good place.

ditto for following "Visible for testing" code

BTW, this method is duplicate to TestUtils#serializeRequestHeader

This method serializes the request with the header while the test method serializes the header only. So, not a duplicate, right? Personally, I think it's useful for the class to be symmetric: it can parse from a buffer and serialize to a buffer. Do you have a concern that it may be misused?

There is a reasonable argument that serializeBody is the counterpart to parse though and this particular method doesn't have to be here. I can buy that.

Do you have a concern that it may be misused?

yes. Also the various serialization methods are a bit chaos to me :(

Maybe we should move all testing-only serialization to a single utils (in test scope). I grep "static ByteBuffer" + "request/response" and MessageUtils, MessageTestUtil, RequestUtils and TestUtils have similar method interface.

I have a refactoring that consolidates the logic around Send. But I think that would be best done in a separate PR since this PR is really massive. Since these methods are all in internal classes and are only used by tests, I suggest we keep them as they are for now and tackle these nits in the follow up PR.

I have these changes locally, so I can submit them immediately after.

chia7712 · 2020-12-04T14:44:42Z


    public String toString(boolean verbose) {
-        return toStruct().toString();
+        return data().toString();


Is this toString variety still useful?

I think so. Do you think it's not?

Sorry, what do you mean by variety? The verbose boolean? Yes, that is useful for things like fetch and produce.

It seems only ProduceRequest does produce verbose string. Maybe we can remove the toString(boolean verbose) as most requests does not generate verbose string. Also, ProduceRequest should override toString() to generate a verbose string always.

The verbosity flag is used for request logging. Produce and fetch are the most important ones. The rest can be ignored. I don't see the benefit in removing this functionality.

chia7712 · 2020-12-04T15:38:58Z

        return data;
    }

+    public void write(ByteBuffer buffer, ObjectSerializationCache serializationCache) {


This is used by testing only. Maybe we can move it to test scope.

chia7712

+1 to this great cleanup and improvement. Please merge it to trunk and then we can address the TODOs in separate PRs :)

ijuma · 2020-12-07T23:40:31Z

Thanks for the review! Will post the follow up in a bit.

dajac · 2020-12-08T08:51:05Z

Nice one @ijuma!

ijuma · 2020-12-09T03:54:42Z

Follow up PR submitted #9714

ijuma requested a review from cmccabe September 29, 2019 03:30

ijuma force-pushed the skip-to-struct-for-generated-requests branch from d4e4903 to a009123 Compare September 30, 2019 03:31

ijuma force-pushed the skip-to-struct-for-generated-requests branch from a44558e to 3106e35 Compare September 30, 2019 12:47

ijuma commented Oct 2, 2019

View reviewed changes

ijuma force-pushed the skip-to-struct-for-generated-requests branch 2 times, most recently from 211d9e0 to 47a197b Compare October 3, 2019 14:59

ijuma added 18 commits October 7, 2019 06:52

Skip redundant toStruct() step when serializing requests that. rely…

7c6ada5

… on the auto generated protocol

Skip redundant toStruct when serializing responses that rely on aut…

bc333a7

…o generated protocol

Add ignorable to TopicAuthorizedOperation and ClusterAuthorizedOperat…

10b01b9

…ions

More ignorable

7f66cda

Add missing whitespace

8be8cef

Move message() to AbstractRequestResponse and document it

a84d3d8

Use toSend instead of serializeWithHeader in `SaslClientAuthentic…

17af66b

…ator`.

Don't rely on Struct directly when parsing requests in the broker

9a93b0b

Avoid intermediate structs when deserializing in the broker

859041a

Avoid intermediate Struct when deserializing responses

98dbf1d

Add back removed tests, modify them to test parsing from buffers inst…

daa6dbb

…ead of structs

Avoid toStruct for request/response header if the request/response …

a36bbb3

…uses the auto generated protocol

Add toString to ResponseHeader

a1bf993

Use diamond

e00cace

Remove unnecessary boxing/unboxing

56cba5f

Use assertThrows to simplify code

5e0f689

Hacky test fix

d6cb097

Add comment

9317347

ijuma added 14 commits December 3, 2020 09:53

Fix NetworkClientTest failures

b17d1bf

Fix KafkaApisTest failures and rename errors to errorsByProducerId fo…

830d46c

…r clarity

Minor style fix

8e719a4

Fix RequestResponseTest and error message in DescribeClientQuotasRequest

de995be

Fix DeleteAclsResponseTest

ce9bd0b

Fix EndTxnResponseTest

d3c8fa6

Remove TestUtils.serializeMessage in favor of MessageTestUtil.message…

b7490af

…ToByteBuffer

Fix LeaveGroupResponseTest

55a6110

Minor fixes

7ad198f

Test fixes/improvements

cfbbede

Fix MetadataRequestTest failure

4829f7c

Use SendBuilder in AbstractRequest/Response

420a74d

Also consolidate and clean-up.

Fix request size returned by parseRequest

cfd0c66

ijuma commented Dec 4, 2020

View reviewed changes

ijuma added 2 commits December 3, 2020 23:05

Revert correlation id verification order change

de6dca1

Revert MetadataResponse.json change

ad0e0d5

ijuma requested a review from chia7712 December 4, 2020 14:42

chia7712 reviewed Dec 4, 2020

View reviewed changes

chia7712 approved these changes Dec 7, 2020

View reviewed changes

ijuma changed the title ~~MINOR: Skip conversion to Struct when serializing generated requests/responses~~ KAFKA-10818: Skip conversion to Struct when serializing generated requests/responses Dec 7, 2020

ijuma merged commit 6f27bb0 into apache:trunk Dec 7, 2020

ijuma deleted the skip-to-struct-for-generated-requests branch December 7, 2020 23:40

This was referenced Dec 13, 2020

MINOR: Skip Struct conversion in FetchRequest.parse #9740

Merged

KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest #9741

Closed

Conversation

ijuma commented Sep 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented Sep 29, 2019

Uh oh!

ijuma commented Sep 30, 2019

Uh oh!

ijuma commented Sep 30, 2019

Uh oh!

ijuma commented Sep 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Oct 2, 2019

Uh oh!

ijuma commented Oct 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Dec 4, 2020

Uh oh!

ijuma commented Dec 4, 2020

Uh oh!

chia7712 left a comment

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!

chia7712 Dec 4, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Dec 7, 2020

Uh oh!

dajac commented Dec 8, 2020

Uh oh!

ijuma commented Dec 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ijuma commented Sep 29, 2019 •

edited

Loading

chia7712 Dec 4, 2020 •

edited

Loading