KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it by brenden20 · Pull Request #16291 · apache/kafka

brenden20 · 2024-06-11T20:01:11Z

I changed OffsetFetchRequestState toString to toStringBase and added a toString() method for MemberInfo

I added a new test to CommitRequestManagerTest that tests the new toStringBase method. In the test, a string variable 'target' is instantiated and will be compared to the new toStringBase method. The test calls OffsetFetchRequestState toStringBase() and compares it to the target string. All tests in CommitRequestManagerTest pass.

brenden20 · 2024-06-11T20:03:05Z

@philipnee @kirktrue new PR for 16557, was having a lot of merge issues

chia7712

@brenden20 thanks for this patch.

chia7712 · 2024-06-11T20:07:30Z

-                    '}';
+        public String toStringBase() {
+            return super.toStringBase() +
+                    ", memberInfo=" + memberInfo +


memberInfo is from super RetriableRequestState. Maybe we should add toStringBase to RetriableRequestState too?

Agree with moving this up to the RetriableRequestState, and once there, I would also suggest we simplify the output to show the member id and epoch directly, something like:

Suggested change

", memberInfo=" + memberInfo +

", memberId=" + memberInfo.memberId.orElse... +

", memberEpoch=" + memberInfo.memberEpoch... +

MemberInfo is just an internal wrapper to move the 2 together, but when seeing a toString for an OffsetFetch or OffsetCommit request, we just care about memberId and memberEpoch (they will be included in the request to the broker)

I have implemented both suggestions

chia7712 · 2024-06-11T20:08:03Z

+        @Override
+        public String toString() {
+            return "MemberInfo{" + "memberId=" + memberId.orElse("undefined") +
+                    ", memberEpoch=" + (memberEpoch.isPresent() ? memberEpoch : "undefined") + "}";


memberEpoch.get?

I would say we need to check if present. There could be no epoch/id when committing/fetching (if we haven't joined a group or left it. In those cases we would have empty here, so that no epoch/id is included in the OffsetFetch/OffsetCommit request).

Agreed! And my point was "we should call get if it is present". Otherwise, the toString will include "Optional"

I implemented this suggestion, I will note that when testing the method, there is no difference in how the string is printed whether using memberEpoch.get() or not

Agreed! And my point was "we should call get if it is present". Otherwise, the toString will include "Optional"

Oh I misread your point he he. But both on the same page in the end, great.

lianetm · 2024-06-11T20:41:28Z

Thanks for the improvement @brenden20 ! Left some comments.

Co-authored-by: Lianet Magrans <98415067+lianetm@users.noreply.github.com>

brenden20 · 2024-06-11T21:23:27Z

@chia7712 @lianetm thank you both for the suggestions! I have implemented them now, let me know what you think!

chia7712

@brenden20 thanks for updated PR. two small comment are left PTAL

chia7712 · 2024-06-11T21:29:45Z

+    @Test
+    public void testOffsetFetchRequestStateToStringBase() {
+        ConsumerConfig config = mock(ConsumerConfig.class);
+        CommitRequestManager.MemberInfo memberInfo = new CommitRequestManager.MemberInfo();


As we had discussion about "Optional#toString", maybe we define either MemberInfo#memberId or MemberInfo#memberEpoch, and then we should check the MemberInfo#toString does not contain "Optional[value]"

I see what you meant now, I have implemented a setter for MemberInfo#memberEpoch and set the memberEpoch value. Upon testing, there is no "Optional[value]"

@brenden20 we already have an onMemberEpochUpdated that you could maybe use for this? so we don't have to add a new setter. That's the actual method that gets called when the member gets a new epoch on the heartbeat response, and we want to pass it on to the commit manager ;)

@lianetm oh I see it now, I will revert that change and use onMemberEpochUpdated instead. Thank you!

chia7712 · 2024-06-11T21:32:36Z

+        public String toStringBase() {
+            return super.toStringBase() +
+                    ", requestedPartitions=" + requestedPartitions +
+                    ", future=" + future;


not sure whether it is worth printing future since it show reference address only :(

Added setter for memberEpoch, removed future from toStringBase

chia7712

@brenden20 thanks for updated PR!

chia7712 · 2024-06-11T22:30:11Z

+
+        assertDoesNotThrow(timedRequestState::toString);
+        assertEquals(target, offsetFetchRequestState.toStringBase());
+    }


Maybe we can add assertFalse(target.contains("Optional")); to make sure we unwrap the optional variables?

chia7712 · 2024-06-11T22:41:31Z

+                retryBackoffMs,
+                retryBackoffMaxMs,
+                1000,
+                memberInfo);


in the line#142 - The memberInfo updated by onMemberEpochUpdated is the inner variable of commitRequestManager. Hence, this memberInfo used to create OffsetFetchRequestState is NOT updated by onMemberEpochUpdated

If we want to complete the test, maybe we can remove MemberInfo from constructor of OffsetFetchRequestState. After all, both RetriableRequestState and OffsetFetchRequestState are the inner class of CommitRequestManager, so it is valid to share the MemberInfo between those classes.

I am a bit hesitant to change the constructor of OffsetFetchRequestState as I do not want to mess up usages elsewhere in the codebase. I did however find a way to achieve updating the inner memberInfo. Test is working as well, I just needed to change the method from private to protected. Let me know what you think

…57-2

chia7712

@brenden20 thanks for updated PR. overall LGTM

chia7712 · 2024-06-12T17:12:48Z


-    private OffsetFetchRequestState createOffsetFetchRequest(final Set<TopicPartition> partitions,
+    // Visible for testing
+    protected OffsetFetchRequestState createOffsetFetchRequest(final Set<TopicPartition> partitions,


maybe package-private is good enough?

Changed to package-private! @chia7712 let me know if there is anything else!

lianetm

LGTM. Thanks for the improvement @brenden20 !

chia7712

LGTM

chia7712 · 2024-06-13T07:30:27Z

@brenden20 thanks for this contribution!

@tag

commit f380cd1 Author: Edoardo Comar <ecomar@uk.ibm.com> Date: Thu Jun 13 15:01:08 2024 +0100 MINOR: Add integration tag to AdminFenceProducersIntegrationTest (apache#16326) Add @tag("integration") to AdminFenceProducersIntegrationTest Reviewers: Chris Egerton <chrise@aiven.io> commit 11c85a9 Author: Dongnuo Lyu <139248811+dongnuo123@users.noreply.github.com> Date: Thu Jun 13 05:11:01 2024 -0400 MINOR: Make online downgrade failure logs less noisy and update the timeouts scheduled in `convertToConsumerGroup` (apache#16290) This patch: - changes the order of the checks in `validateOnlineDowngrade`, so that only when the last member using the consumer protocol leave and the group still has classic member(s), `online downgrade is disabled` is logged if the policy doesn't allow downgrade. - changes the session timeout in `convertToConsumerGroup` from `consumerGroupSessionTimeoutMs` to `member.classicProtocolSessionTimeout().get()`. Reviewers: David Jacot <djacot@confluent.io> commit ea60666 Author: Ken Huang <100591800+m1a2st@users.noreply.github.com> Date: Thu Jun 13 17:11:37 2024 +0900 KAFKA-16921 [1/N] Migrate all junit 4 code to junit 5 for connect module (apache#16253) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 596b945 Author: gongxuanzhang <gongxuanzhang@foxmail.com> Date: Thu Jun 13 15:39:32 2024 +0800 KAFKA-16643 Add ModifierOrder checkstyle rule (apache#15890) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 103ff5c Author: Antoine Pourchet <antoine@responsive.dev> Date: Thu Jun 13 01:32:39 2024 -0600 KAFKA-15045: (KIP-924 pt. 24) internal TaskAssignor rename to LegacyTaskAssignor (apache#16318) Since the new public API for TaskAssignor shared a name, this rename will prevent users from confusing the internal definition with the public one. Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org> commit e59c887 Author: brenden20 <118419078+brenden20@users.noreply.github.com> Date: Thu Jun 13 02:30:05 2024 -0500 KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it (apache#16291) Reviewers: Lianet Magrans <lianetmr@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com> commit dd6fcc6 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 14:35:33 2024 +0800 KAFKA-16901 Add unit tests for ConsumerRecords#records(String) (apache#16227) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit fe98888 Author: Lianet Magrans <98415067+lianetm@users.noreply.github.com> Date: Thu Jun 13 08:31:16 2024 +0200 MINOR: Improving log for outstanding requests on close and cleanup (apache#16304) Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com> commit 9ddd58b Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 05:43:33 2024 +0200 MINOR: Add readiness check for connector and separate Kafka cluster in ExactlyOnceSourceIntegrationTest::testSeparateOffsetsTopic (apache#16306) Reviewers: Greg Harris <gharris1727@gmail.com> commit 0a203a9 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 09:47:51 2024 +0800 KAFKA-16938 non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. (apache#16302) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 6d1f8f8 Author: Gantigmaa Selenge <39860586+tinaselenge@users.noreply.github.com> Date: Thu Jun 13 02:42:39 2024 +0100 MINOR: Clean up for KafkaAdminClientTest (apache#16285) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit e76e1da Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 02:18:23 2024 +0200 KAFKA-16935: Automatically wait for cluster startup in embedded Connect integration tests (apache#16288) Reviewers: Greg Harris <gharris1727@gmail.com>

@tag

commit 4333af5 Author: A. Sophie Blee-Goldman <ableegoldman@gmail.com> Date: Thu Jun 13 11:27:50 2024 -0700 KAFKA-15045: (KIP-924 pt. 25) Rename old internal StickyTaskAssignor to LegacyStickyTaskAssignor (apache#16322) To avoid confusion in 3.8/until we fully remove all the old task assignors and internal config, we should rename the old internal assignor classes like the StickyTaskAssignor so that they won't be mixed up with the new version of the assignor (which is also named StickyTaskAssignor) Reviewers: Bruno Cadonna <cadonna@apache.org>, Josep Prat <josep.prat@aiven.io> commit f380cd1 Author: Edoardo Comar <ecomar@uk.ibm.com> Date: Thu Jun 13 15:01:08 2024 +0100 MINOR: Add integration tag to AdminFenceProducersIntegrationTest (apache#16326) Add @tag("integration") to AdminFenceProducersIntegrationTest Reviewers: Chris Egerton <chrise@aiven.io> commit 11c85a9 Author: Dongnuo Lyu <139248811+dongnuo123@users.noreply.github.com> Date: Thu Jun 13 05:11:01 2024 -0400 MINOR: Make online downgrade failure logs less noisy and update the timeouts scheduled in `convertToConsumerGroup` (apache#16290) This patch: - changes the order of the checks in `validateOnlineDowngrade`, so that only when the last member using the consumer protocol leave and the group still has classic member(s), `online downgrade is disabled` is logged if the policy doesn't allow downgrade. - changes the session timeout in `convertToConsumerGroup` from `consumerGroupSessionTimeoutMs` to `member.classicProtocolSessionTimeout().get()`. Reviewers: David Jacot <djacot@confluent.io> commit ea60666 Author: Ken Huang <100591800+m1a2st@users.noreply.github.com> Date: Thu Jun 13 17:11:37 2024 +0900 KAFKA-16921 [1/N] Migrate all junit 4 code to junit 5 for connect module (apache#16253) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 596b945 Author: gongxuanzhang <gongxuanzhang@foxmail.com> Date: Thu Jun 13 15:39:32 2024 +0800 KAFKA-16643 Add ModifierOrder checkstyle rule (apache#15890) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 103ff5c Author: Antoine Pourchet <antoine@responsive.dev> Date: Thu Jun 13 01:32:39 2024 -0600 KAFKA-15045: (KIP-924 pt. 24) internal TaskAssignor rename to LegacyTaskAssignor (apache#16318) Since the new public API for TaskAssignor shared a name, this rename will prevent users from confusing the internal definition with the public one. Reviewers: Anna Sophie Blee-Goldman <ableegoldman@apache.org> commit e59c887 Author: brenden20 <118419078+brenden20@users.noreply.github.com> Date: Thu Jun 13 02:30:05 2024 -0500 KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it (apache#16291) Reviewers: Lianet Magrans <lianetmr@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com> commit dd6fcc6 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 14:35:33 2024 +0800 KAFKA-16901 Add unit tests for ConsumerRecords#records(String) (apache#16227) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit fe98888 Author: Lianet Magrans <98415067+lianetm@users.noreply.github.com> Date: Thu Jun 13 08:31:16 2024 +0200 MINOR: Improving log for outstanding requests on close and cleanup (apache#16304) Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com> commit 9ddd58b Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 05:43:33 2024 +0200 MINOR: Add readiness check for connector and separate Kafka cluster in ExactlyOnceSourceIntegrationTest::testSeparateOffsetsTopic (apache#16306) Reviewers: Greg Harris <gharris1727@gmail.com> commit 0a203a9 Author: TingIāu "Ting" Kì <kitingiao@gmail.com> Date: Thu Jun 13 09:47:51 2024 +0800 KAFKA-16938 non-dynamic props gets corrupted due to circular reference between DynamicBrokerConfig and DynamicConfig. (apache#16302) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit 6d1f8f8 Author: Gantigmaa Selenge <39860586+tinaselenge@users.noreply.github.com> Date: Thu Jun 13 02:42:39 2024 +0100 MINOR: Clean up for KafkaAdminClientTest (apache#16285) Reviewers: Chia-Ping Tsai <chia7712@gmail.com> commit e76e1da Author: Chris Egerton <chrise@aiven.io> Date: Thu Jun 13 02:18:23 2024 +0200 KAFKA-16935: Automatically wait for cluster startup in embedded Connect integration tests (apache#16288) Reviewers: Greg Harris <gharris1727@gmail.com>

Add toStringBase(), toString(), and a test

81a5572

brenden20 mentioned this pull request Jun 11, 2024

KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it in CommitRequestManagerTest #16115

Closed

chia7712 reviewed Jun 11, 2024

View reviewed changes

lianetm reviewed Jun 11, 2024

View reviewed changes

Comment thread clients/src/test/java/org/apache/kafka/clients/consumer/internals/CommitRequestManagerTest.java Outdated

brenden20 and others added 3 commits June 11, 2024 15:54

Cleaner line

7147a27

Co-authored-by: Lianet Magrans <98415067+lianetm@users.noreply.github.com>

Add toStringBase() to RetriableRequestState

e91891f

Remove print statement

caf17bb

chia7712 reviewed Jun 11, 2024

View reviewed changes

brenden20 added 2 commits June 11, 2024 16:43

Small update

1941a55

Added setter for memberEpoch, removed future from toStringBase

Removed unnecessary setter method

5768017

chia7712 reviewed Jun 11, 2024

View reviewed changes

brenden20 and others added 4 commits June 12, 2024 08:55

Merge branch 'apache:trunk' into 16557-2

94f26b5

Add assertFalse statement

837d9ea

Fixed toStringBase() test

98361ea

Merge branch '16557-2' of https://github.com/brenden20/kafka into 165…

f061d97

…57-2

chia7712 reviewed Jun 12, 2024

View reviewed changes

Changed method visibility

2583e12

lianetm approved these changes Jun 12, 2024

View reviewed changes

chia7712 approved these changes Jun 13, 2024

View reviewed changes

chia7712 merged commit e59c887 into apache:trunk Jun 13, 2024

brenden20 deleted the 16557-2 branch June 13, 2024 15:35

	", memberInfo=" + memberInfo +
	", memberId=" + memberInfo.memberId.orElse... +
	", memberEpoch=" + memberInfo.memberEpoch... +

Conversation

brenden20 commented Jun 11, 2024

Uh oh!

brenden20 commented Jun 11, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm Jun 11, 2024 • 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!

Uh oh!

lianetm commented Jun 11, 2024

Uh oh!

brenden20 commented Jun 11, 2024

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!

lianetm Jun 11, 2024 • 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!

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!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brenden20 Jun 12, 2024 • 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!

lianetm left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Jun 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

lianetm Jun 11, 2024 •

edited

Loading

lianetm Jun 11, 2024 •

edited

Loading

brenden20 Jun 12, 2024 •

edited

Loading