MINOR: remove FetchResponse.AbortedTransaction and redundant construc… by chia7712 · Pull Request #9758 · apache/kafka

chia7712 · 2020-12-16T08:52:35Z

Changes

rename INVALID_HIGHWATERMARK to INVALID_HIGH_WATERMARK
replace FetchResponse.AbortedTransaction by FetchResponseData.AbortedTransaction
remove redundant constructors from FetchResponse.PartitionData
rename recordSet to records

Performance Tests

loop 10 times and get average.

case 1: @parametrize(acks=1, topic=TOPIC_REP_ONE)

diff: -0.3994016685 %

trunk: 64.847 MB/sec
patch: 64.588 MB/sec

case 2: @parametrize(acks=-1, topic=TOPIC_REP_THREE)

diff: +0.4917916785 %

trunk: 28.264 MB/sec
patch: 28.403 MB/sec

Committer Checklist (excluded from commit message)

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

ijuma

Thanks for the PR. A couple of initial comments.

chia7712 · 2021-01-06T07:02:12Z

I don't batch the partitions again in this PR as it create a new FetchResponseData.

Can you clarify what you mean here?

Oh, I planed to remove all usages of this method from production (i.e KafkaApis should generate batched response directly). However, it can produce a big patch so I will keep this method in next commit.

chia7712 · 2021-01-11T14:08:39Z

fix conflicting files

…tors of PartitionData

chia7712 · 2021-01-13T13:56:09Z

fix conflicting files again

ijuma

Thanks for the updates. A few more comments below.

…d to get records size

chia7712 · 2021-03-02T04:17:28Z

@ijuma Thanks for all your great comments. I have updated this PR. Please take a look.

ijuma

Thanks for the updates. I think these are the last comments and then we should be good. :)

chia7712 · 2021-03-02T16:23:26Z

@ijuma thanks for your comments. I have addressed them in latest commit (55b35ab). Please take a look.

ijuma

I left one comment, the rest LGTM.

chia7712 · 2021-03-04T10:00:10Z

Build / JDK 11 / org.apache.kafka.connect.integration.BlockingConnectorTest.testWorkerRestartWithBlockInConnectorStop

unrelated flaky

ijuma · 2021-03-04T13:22:19Z

Great to see this merged. @chia7712 Will you follow up with the items we split into separate JIRAs? I think the 3 key ones are:

Remove unnecessary copying, if possible, in KafkaApis
Remove the lazily populated map in FetchResponse
I think you had an idea for removing FetchResponse.toMessage

chia7712 · 2021-03-04T13:28:03Z

Remove unnecessary copying, if possible, in KafkaApis

https://issues.apache.org/jira/browse/KAFKA-12385

Remove the lazily populated map in FetchResponse

https://issues.apache.org/jira/browse/KAFKA-12387

I think you had an idea for removing FetchResponse.toMessage

ugh, I missed this one. opened: https://issues.apache.org/jira/browse/KAFKA-12410

jolshan · 2021-03-08T21:14:29Z

@chia7712 I noticed that all of these JIRAs are listed as open, but they reference this PR. Are there further changes to be made? Asking because I'm working on similar code for #9944 and want to minimize merge conflicts as best as possible.

chia7712 · 2021-03-09T03:59:06Z

I noticed that all of these JIRAs are listed as open, but they reference this PR. Are there further changes to be made? Asking because I'm working on similar code for #9944 and want to minimize merge conflicts as best as possible.

thanks for this response. Only #10269 is in progress. I'm sorry that this PR brings a bunch of conflicting files to #9944. I'd like to merge #9944 before other minor refactor-related patches.

jolshan · 2021-03-09T15:21:24Z

@chia7712 sounds good. So is the plan to merge #10269 first or after #9944? Also thanks for making these changes. I noticed a lot of this when working on #9944

chia7712 · 2021-03-09T15:24:23Z

So is the plan to merge #10269 first or after #9944?

yep. #10269 is in progress (tweak performance). I will take a look at #9944 :)

ijuma · 2021-03-09T16:42:42Z

Does the refactoring simplify #9944 or not?

jolshan · 2021-03-09T17:25:41Z

@ijuma on first glance, I'm not sure if it simplifies. But it will introduce changes. And these are likely changes we want to have eventually.

jolshan · 2021-03-09T19:07:45Z

So should we go forward with #10269 first still?

chia7712 · 2021-03-09T19:14:12Z

So should we go forward with #10269 first still?

It needs some time to complete performance tests. It seems to me we can fix conflicting files in #9944 to get reviews and then push it before merging #10269. BTW, don't worry about causing potential conflicting files to #10269 :)

jolshan · 2021-03-09T19:21:08Z

@chia7712 I see. Thanks for the response. I'm not sure if #9944 will be ready after fixing conflicts. There were still a few critical tests failing. I can try to fix those but still figuring out the timeline. I can go ahead and start with the conflicts though.

chia7712 requested review from dajac, hachikuji and ijuma December 16, 2020 08:52

chia7712 changed the title ~~MINOR: remove FetchResponse.AbortedTransaction and duplicate construc…~~ MINOR: remove FetchResponse.AbortedTransaction and redundant construc… Dec 16, 2020

chia7712 force-pushed the MINOR-9758 branch 2 times, most recently from 0fe301e to bae931b Compare December 19, 2020 06:35

chia7712 commented Dec 19, 2020

View reviewed changes

Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated

chia7712 force-pushed the MINOR-9758 branch 2 times, most recently from 2a3ce40 to 3b606da Compare December 29, 2020 07:25

ijuma reviewed Jan 4, 2021

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java Outdated

Comment thread clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java Outdated

Comment thread clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java Outdated

chia7712 force-pushed the MINOR-9758 branch 2 times, most recently from 86da652 to 1a5def5 Compare January 5, 2021 20:03

chia7712 commented Jan 6, 2021

View reviewed changes

ijuma reviewed Jan 6, 2021

View reviewed changes

Comment thread core/src/main/scala/kafka/server/FetchSession.scala Outdated

chia7712 force-pushed the MINOR-9758 branch from 5fa28b5 to 665e6c9 Compare January 11, 2021 14:08

chia7712 added 5 commits January 13, 2021 21:50

MINOR: remove FetchResponse.AbortedTransaction and duplicate construc…

b3e592f

…tors of PartitionData

remove unnecessary changes and remove partitionData

f217bb5

remove PartitionData

3f219e5

add isDivergingEpoch and isPreferredReplica

83a26f0

remove unused import

7c386a0

chia7712 force-pushed the MINOR-9758 branch from 665e6c9 to 7c386a0 Compare January 13, 2021 13:56

chia7712 added 7 commits January 14, 2021 14:16

Merge branch 'trunk' into MINOR-9758

c010e4e

Merge branch 'trunk' into MINOR-9758

30695f8

Merge branch 'trunk' into MINOR-9758

9122aee

Merge branch 'trunk' into MINOR-9758

4106d2f

fix checkstyle

d97de8a

Merge branch 'trunk' into MINOR-9758

13f7710

tweak style

52ab156

Merge branch 'trunk' into MINOR-9758

033b933

ijuma reviewed Mar 1, 2021

View reviewed changes

chia7712 added 2 commits March 2, 2021 11:14

Merge branch 'trunk' into MINOR-9758

8263bda

remove redundant code; add more comment for casting; use helper metho…

529d81d

…d to get records size

ijuma reviewed Mar 2, 2021

View reviewed changes

chia7712 added 3 commits March 2, 2021 23:32

Merge branch 'trunk' into MINOR-9758

9f8e97b

fix build error

4fa69c5

set partition index; add recordsSize;

55b35ab

jolshan mentioned this pull request Mar 3, 2021

KAFKA-10580: Add topic ID support to Fetch request #9944

Merged

3 tasks

ijuma reviewed Mar 4, 2021

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java Outdated

ijuma approved these changes Mar 4, 2021

View reviewed changes

chia7712 added 2 commits March 4, 2021 15:29

Merge branch 'trunk' into MINOR-9758

821c307

revert sizeOf

ae25551

chia7712 merged commit 8205051 into apache:trunk Mar 4, 2021

dengziming mentioned this pull request Mar 30, 2021

MINOR: Remove wrong suppress #10434

Merged

3 tasks

chia7712 deleted the MINOR-9758 branch March 25, 2024 15:21

Conversation

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

Uh oh!

Changes

Performance Tests

case 1: @parametrize(acks=1, topic=TOPIC_REP_ONE)

case 2: @parametrize(acks=-1, topic=TOPIC_REP_THREE)

Committer Checklist (excluded from commit message)

Uh oh!

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chia7712 Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

ijuma Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

chia7712 Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chia7712 commented Jan 11, 2021

Uh oh!

chia7712 commented Jan 13, 2021

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chia7712 commented Mar 2, 2021

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chia7712 commented Mar 2, 2021

Uh oh!

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Mar 4, 2021

Uh oh!

ijuma commented Mar 4, 2021

Uh oh!

chia7712 commented Mar 4, 2021

Uh oh!

jolshan commented Mar 8, 2021

Uh oh!

chia7712 commented Mar 9, 2021

Uh oh!

jolshan commented Mar 9, 2021

Uh oh!

chia7712 commented Mar 9, 2021

Uh oh!

ijuma commented Mar 9, 2021

Uh oh!

jolshan commented Mar 9, 2021

Uh oh!

jolshan commented Mar 9, 2021

chia7712 commented Dec 16, 2020 •

edited

Loading

jolshan commented Mar 9, 2021 •

edited

Loading